comment out unsupported defines, and perhaps fix some

[ctzsnooze]

While testing this grep string tp find the defines we use,

grep --exclude-dir=config -Irh '^[^/].*' src/ | grep -wo 'USE_[0-9A-Z_]\+' | sort | uniq

If we remove the exclusion on the config directory, a whole lot of config files can be seen to reference defines which do not exist. Some are being changed to existing defines that should work, others are commented out pending a formal resolution of what should be done with them.

Changed defines:

From	To	Boards
USE_I2C1_PULLUP ON	I2C1_PULLUP ON	Alienflight
USE_I2C3_PULLUP ON	I2C3_PULLUP ON	RacePit
USE_FLASH_W25Q128	USE_FLASH_W25Q128FV	SPRacingH7EF @hydra ??
USE_FLASH_W25P16	USE_FLASH_M25P16	BluejayF4, CycloneF722
USE_FLASH_W25Q256	USE_FLASH_W25Q128FV	BrahmaF405

Commented out defines:

Define	Reason	Boards
USE_RX_EXPRESSLRS_TELEMETRY	no such define	lots of boards @Phobos21 ??
USE_ACCGYRO_ASM330LHH	no such define	NeutronRCF435SE
USE_FAKE_ACC	no such define	NucleoF446
USE_FAKE_GYRO	no such define	NucleoF446
USE_FAKE_MAG	no such define	NucleoF446
USE_SONAR	no such define	NucleoF446
USE_CC2500	no such define	NucleoF446
USE_MPU_DATA_READY_SIGNAL	no such define	SPRacingF7EVO @hydra ??
USE_GYRO_FAST_KALMAN	no such define	YupiF4

Please carefully check that these are sensible changes :-)

[haslinghuis]

Thanks for cleaning up Chris.

• #define USE_RX_EXPRESSLRS_TELEMETRY should stay
• #define USE_FLASH_W25P16 shoud probably be USE_FLASH_M25P16
• #define USE_FLASH_W25Q256 should probably be USE_FLASH_W25Q128FV
• NUCLEOF446 is a development board.
• #define USE_MPU_DATA_READY_SIGNAL should probably be USE_MAG_DATA_READY_SIGNAL

[nerdCopter]

* #define `USE_MPU_DATA_READY_SIGNAL` should probably be `USE_MAG_DATA_READY_SIGNAL`

USE_MPU_DATA_READY_SIGNAL used to be a legitimate define, at least in 3.5.x. It's probably not _MAG_.
Maybe @hydra can clarify. (ref ./configs/SPRACINGH7EVO/config.h:116)

[ctzsnooze]

@haslinghuis @nerdCopter thanks for the advice. For now I've made some updates and summarised them in the tables above.

• I'm still not clear why we would keep the USE_RX_EXPRESSLRS_TELEMETRY define active when it isn't used in 4.4 or 4.5. I wonder if someone with knowledge of ELRS - @CapnBry or @Phobos21 perhaps - could advise on this?
• I've put USE_FLASH_W25Q128FV in place of USE_FLASH_W25Q256 but have asked @SteveCEvans advice on this. Would be good if the drivers had a comment that explained which chips each driver supported.

[ctzsnooze]

Here are a few defines for which I can't figure out where they get 'defined'... so long as that's OK, I'm good with it:

Define	Code	Concern
USE_ADC3_DIRECT_HAL_INIT	stm32h7xx.c	used but not set
USE_ADC_INTERRUPT	adc_stm32g4xx.c, adc_stm32h7xx.c	used but not set
USE_BIND_ADDRESS_FOR_DATA_STATE	nrf24_inav.c	used but define line is commented out
USE_BRUSHED	msp_build_info.c	sets a build option, but never defined
USE_BST	tasks.c, scheduler.h	used to include i2c_bst.h but never defined
USE_CCM_CODE	system.c, common_post.h	never defined
USE_HAL_xxx_xxx	stm32h7xx_hal_conf.h	local to that file, not really user defines

[haslinghuis]

@ctzsnooze agree #define USE_RX_EXPRESSLRS_TELEMETRY can be removed.

[CapnBry]

Hey Betaflight crew, I'm out of town, don't have my glasses, and haven't looked at the ELRS SPI code in nearly a year, but I'd be happy to take a close look at comment Sunday night/ Monday morning.

[CapnBry]

I can also confirm USE_RX_EXPRESSLRS_TELEMETRY has never been used, not even in the original code for ExpressLRS 2.x SPI, so it should be removed.

[hydra]

IIRC USE_MPU_DATA_READY_SIGNAL was used so that FC's would fail gyro checks on boot if the INT pin wasn't soldered on correctly, prior to support for EXTI was added, wasn't enabled by default as some old FC's didn't have the gyro INT signal connected to the MCU.

Not sure what's happened to this functionality but if it's been removed it should be restored so that users and factories can be sure their gyros are working correctly.

EDIT:

ah, I was thinking of ENSURE_MPU_DATA_READY_IS_LOW.

that probably should be cleaned up somewhat so that that it's always used, and should be based on the appropriate INT signal level for the gyro being used, after it's correctly detected.

MPU int signal level failure should result in some form of error or status that can be retieved.

Perhaps this ^^^ should be moved into a separate issue so it can be properly tracked and planned?

Also, ENSURE_MPU_DATA_READY_IS_LOW could probably be tied to the gating for all the gyros like ICM20602 and later, as IIRC at that point all the FCs should have been using the INT signal. And if they don't, perhaps plan to make them unsupported for the next release.

Not sure if the hardware guidelines have a note regarding the requirement for the gyro int signal, but if not it should probably be added and pin selection for gyro int signals should either be on a gpio pin with a dedicated interrupt, e.g. EXTI1/EXTI2/EXTI3/EXTI/4, and not EXTI9_5 or EXTI15_10 which should only be used for lower frequency interrupts due to the additional overhead in determining which exti signal caused the interrupt.

[hydra]

USE_FLASH_W25Q128 USE_FLASH_W25Q128FV

Yes, this is fine. IIRC this came about around the time I was working on QSPI support for the W25Q128 flash, but the EF doesn't have the octospi drivers enabled as the octospi usage is invisible to BF on this target because it boots and runs firmware from octospi but BF stores config on the 2nd flash chip which is connected via SPI. Making the change should be tested by a maintainer with access to an H7EF. Please confirm this has been done before merging this PR. IIRC @haslinghuis has one.

[haslinghuis]

@hydra this is without the change:

# Betaflight / STM32H730 (SP7E) 4.6.0 May 13 2024 / 14:27:18 (2d3c8eea3) MSP API: 1.46
# config rev: 45995c5
# board: manufacturer_id: SPRO, board_name: SPRACINGH7EF

MCU H730 Clock=520MHz, Vref=3.34V, Core temp=55degC
Stack size: 2048, Stack address: 0x20020000
Configuration: CONFIGURED, size: 3839, max available: 65536
Devices detected: SPI:2, I2C:0
Gyros detected: gyro 1, gyro 2 locked dma
GYRO=ICM42688P, ACC=ICM42688P
OSD: MSP (30 x 13)
BUILD KEY: c39e18bc95a96534881c96c161244871 (4.6.0-dev)
System Uptime: 29 seconds, Current Time: 2024-05-13T14:29:11.795+00:00
CPU:37%, cycle time: 121, GYRO rate: 8264, RX rate: 15, System rate: 9
Voltage: 301 * 0.01V (1S battery - CRITICAL)
I2C Errors: 10
FLASH: JEDEC ID=0x00ef4015 2M
GPS: NOT ENABLED
Arming disable flags: RXLOSS ANGLE CLI MSP

[hydra]

Define Code Concern
USE_BRUSHED msp_build_info.c sets a build option, but never defined

probably dates back to the CJMCU brushed board from Cleanflight.

Define Code Concern
USE_BST tasks.c, scheduler.h used to include i2c_bst.h but never defined

probably dates back to the TBS PowerCube stack and related targets.

Define Code Concern
USE_CCM_CODE system.c, common_post.h never defined

probably no-longer exists due to removal of F3 support which used Core Coupled Memory (CCM).

[hydra]

@hydra this is without the change:

FLASH: JEDEC ID=0x00ef4015 2M

yes, this is what you're looking for after you make the change, likely though it just wouldn't boot due to having no-where to store the config if the change didn't work.

The board you have has a W25Q16JV fitted:

[haslinghuis]

@hydra

yes, this is what you're looking for after you make the change, likely though it just wouldn't boot due to having no-where to store the config if the change didn't work.

This is before the change. So it seems W25Q128FV define is not needed?

[hydra]

@hydra

yes, this is what you're looking for after you make the change, likely though it just wouldn't boot due to having no-where to store the config if the change didn't work.

This is before the change. So it seems W25Q128FV define is not needed?

seems not then; due to the octospi reasons I mentioned before, in which case it can be removed from the target. As and when I have time to work on octospi and spi I can make corresponding target updates so that BF stores it's config on the memory mapped flash so that the entire SPI flash is used for logging.

[nerdCopter]

i'll approve this if the single suggestion is resolved. review by eye looks good

[ctzsnooze]

Thanks @haslinghuis @nerdCopter for reviewing this PR. I'd approve :-) but it's my PR so I can't.