sx126x: Reordering of mode state tracker updates breaks SX126xCheckDeviceReady

[andysan]

I just started to test the commits from #926 and it seems like it had some unintended consequences. Particularly, it seems like 97076b0 causes the radio to be stuck in sleep mode instead of entering RX mode when joining a network.

I haven't debugged this properly, but it seems like this is caused by SX126xCheckDeviceReady not realising that it needs to wake the radio.

[mluis1]

Thanks for the feedback.

It was then a bad idea to re-order the calls. I'll revert the changes

[andysan]

I'm not entirely sure what the best solution would be here. The RF switch probably won't like it if the chip enters TX mode while RX path is enabled. On the other hand, there is no simple way of waking the chip if the mode is updated before issuing the relevant commands.

It seems to me like there are two different states that need to be tracked here, one is the sleep state and the other one is the RF path that will be activated. It might be worth tracking them separately to make sure we don't fry the RF switch.

[mluis1]

I guess that we can revert the ordering for all commands. The change was done because you mentioned that potentially a high power would be applied to a non itialized antenna switch.

Actually, I think that this can only happen for a very short time period which shouldn't make a big difference to the antenna switch. It has to be noted that the full RF power is not applied directly, the radio automatically ramps-up the power so, it takes some time to have full RF power applied to the RF output.

I would like to propose that we revert back the calls order up until a better way is found.

[andysan]

I think you are right for the majority of cases. However, the thread could get preempted for some reason between the SPI command and the RF switch update. That could, at least in theory, lead to an arbitrary delay before the switch is configured.

[mluis1]

I see, then for OS operation we could introduce a critical section which would prevent the OS to preempt between the 2 operations.

[andysan]

I’m not convinced that would work either since many chips use DMA for SPI transactions and that’ll require interrupts to be enabled.

[mluis1]

I'll check with my colleagues who are RF experts what happens if RF power is applied to an antenna switch setup for Rx path.

To solve this situation for OS support some of the possible solutions are

1. Use the radio automatic handling of the RF switch. In case the RF switch requires more than one control pin a NOT gate could be use to generate the inverted radio control signal.

Radio-Rx/Tx ----+----------- AntSw-Tx
                |
                +----|>o---- AntSw-Rx

2. Figure out a better way to handle this by refactoring the operating mode and antenna switch control. (requires some time)
3. For OS operation find a way to prevent OS preemption while accessing the radio. For the moment I have no idea on how we could make this happen.
4. Other ideas?

In the mean time I will revert the calls ordering.

[andysan]

1 and 3 are probably not going to work in practice. Changing the hardware is sadly not an option in many cases. Many modules already assume that the switch can be controlled by software and I would like to be able to support them in Zephyr. Preventing OS prevention and at the same time having the ability to perform DMA and take interrupts doesn't seem feasible.

In reality, the RF switch might be able to cope with 20dBm for a second or so, so it'll probably not fry the board, so we are probably OK with the old order of maintaining state. I'm not an RF engineer though, so I don't have the experience to judge that.

The best long-term solution might be to change the SX126xAntSwOff/SX126xAntSwOn API and replace it with a general SX126xAntSwSetState API that takes an enum (off/rx/tx) instead of overloading the state tracker with control functionality.

[mluis1]

Thanks for the feedback.

I will try to implement option 2 as soon as possible following your recommendation.

When I receive the feedback from my colleagues I'll update this issue.

[andysan]

Thanks for the feedback.

I will try to implement option 2 as soon as possible following your recommendation.

When I receive the feedback from my colleagues I'll update this issue.

Excellent, thanks!

[andysan]

I got some time to have another look at the HAL today, and it seems like a simple solution might be possible. In fact, the current version (after reverting the change that reordered mode updates) HAL mostly does the right thing to track the RF switch state.

My understanding is that the desired behaviour is as follows:

• Transition to RX: update after SPI command (TX->RX is an allowed transition, which means that we need to disable transmission before disabling the TX path in the switch)
• Transition to TX: update before SPI command (switch need to be configured for TX before enabling TX)
• Transition to idle/sleep: update after SPI command (ensures there is no TX when updating the switch)

The HAL currently does the following:

• SX126xSetSleep/SX126xSetStandby/SX126xSetFs: After
• SX126xSetTx: Before
• SX126xSetTxContinuousWave / SX126xSetTxInfinitePreamble: Before
• SX126xSetRx/SX126xSetRxBoosted: Before (this could be a problem if TX->RX is a valid transition)
• SX126xSetRxDutyCycle / SX126xSetCAD: After

Updating the tracker before sending the command seems like it would be legal since you would never come from the sleep state where an explicit wake-up is needed.

With the exception of the CW and infinite preamble calls (and potentially RX?), I think this could work. I need to re-test my Zephyr driver though.

[mluis1]

Thanks for the analysis.

Once you have finalized your tests would you mind to create a pull-request with required changes?

In the long term I think that your proposition to split the antenna control fro the operating mode control would be the most robust solution. As soon as I get some time I'll implement this change.

[mluis1]

We are currently updating all radio drivers to newer versions.
I guess that we have addressed all the concerns provided in this issue.

@andysan Could you please take a look at v5.0.0-branch in order to confirm that the made changes address your concerns.
So far we successfully have run Class A pre-certification tests using every supported radio (SX126x, SX127x and LR1110).

For further information about the changes that will be introduced for v5.0.0 release please refer to #1239 discussion topic.

If you don't mind I convert this issue to a discussion topic.