Fixed SPI mode enum in SubGHz library

[jgromes]

Hello, I found this issue in the following CI run for RadioLib: https://github.com/jgromes/RadioLib/actions/runs/6914863886/job/18813152408#step:8:28

Summary

This PR fixes unknown SPI mode in SubGHz library (changed in 392469a).

[fpistm]

Thanks @jgromes
I've tested your library when done the major part of the rework. And last I've changed this enum.
Sorry for the trouble and thanks for the fix.

[jgromes]

@fpistm glad I could help! Any estimate on when is the new version going to be released?

[fpistm]

@fpistm glad I could help! Any estimate on when is the new version going to be released?

Hi @jgromes
I will do a 2.7.1 release for bugfix.
Anyway, this PR and #2193 is not correct.
Proper way to define is:
SPISettings(16000000, MSBFIRST, SPI_MODE0)

I will do a proper fixes on core side, anyway the new SPISettings aligned with the ArduinoCore-API does not work with RadioLib.
It seems the values is not evaluated when the STM32WLx radio = new STM32WLx_Module(); is done.
SubGhz.spi_settings is not evaluate yet:
https://github.com/jgromes/RadioLib/blob/81c59f61ff69749be9579a3566dc55d3bfb765b8/src/modules/SX126x/STM32WLx_Module.cpp#L27C25-L27C25

Replacing by SPISettings(16000000, MSBFIRST, SPI_MODE0) solve the problem.
I have to dig into this to propose a proper fix.
Maybe I will have to do a PR to RadioLib. If you have any idea do not hesitate. 😜

[jgromes]

@fpistm

It seems the values is not evaluated when the STM32WLx radio = new STM32WLx_Module(); is done.

Not sure what you mean - I can compile RadioLib STM32WL example fine with the changes from both PRs.

[fpistm]

@fpistm

It seems the values is not evaluated when the STM32WLx radio = new STM32WLx_Module(); is done.

Not sure what you mean - I can compile RadioLib STM32WL example fine with the changes from both PRs.

It compiles but does not work.

[jgromes]

Ah, I see. Odd, I can't test that unfortunately, since I don't have any STM32WL on hand. Maybe @matthijskooijman would be able to help?

[matthijskooijman]

I've read up a bit and am trying to make sense of the changes. I think that:

• 392469a Syncs the SPISettings API with ArduinoCore-API, adding some methods and changing the initialization. This makes the constructor no longer constexpr, and removes the SPI_MODE_x constants (which actually were an implementation detail, the actual API should have used SPI_MODEx, but since the values were the same using the wrong enum did actually work).
• fix(SubGhz): SPISettings not properly defined #2193 changes the SubGhz SPISettings object to not be constexpr, which is no longer possible now SPISettings constructor is not constexpr anymore.
• This PR changes the use of SPI_MODE_x to SPI_MODEx so it uses the correct API.

Then, what @fpistm is saying is that by the time radiolib uses SubGhz.spi_settings, it is now actually initialized yet. This is because both are part of global object initialization, and there is no guarantee about relative ordering of global objects in different compilation units. This was not previously a problem, since constexpr objects are evaluated at compiletime and are initialized before everything else (by copying bytes from the .data segment instead of running code).

Wrt to 392469a, I wonder if all these changes are really needed. Personally, I would like to keep the constructor constexpr, which ensures that constructing an SPISettings object can be done at compiletime. The entire alwaysInline/mightInline stuff really stems from the AVR implementation, where this was, IIRC, needed because there constructing an SPISettings object would condense the settings into a single uint8_t value ready to be put into the hardware registers directly, to ensure that the SPISettings approach was just as compact as the old (non-portable) method of specifying the register values directly. I think that approach also predates the constexpr concept, so maybe the inline stuff isn't actually really needed in AVR anymore either.

In any case - I think the inline stuff has no place in ArduinoCore-API, since it really does not add anything (in fact, I wonder about the definition of this class in the API repo at all, since any core that includes ArduinoCore-API now can no longer define an optimized SPISettings class like AVR does now - I think ArduinoCore-API should just define an expected API and let cores implement that however they want).

So I would suggest stripping the alwaysInline/mightInline stuff from SPISettings - it adds nothing, doing the intialization directly in the constructor and making it constexpr again. Then reverting #2193 and keeping this PR should, I think make things work as before.

I've got to run now, but if it is helpful I could have a closer look and/or test somewhere next week?

[fpistm]

Thanks @matthijskooijman.
Agreed, that's the way I thought to fix the issue and avoid other regressions.

@jgromes could you file an issue to revert the SPISettings as constexpr, please ?