Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delay after CRSFconnect before TX transmit starts #1775

Merged
merged 4 commits into from Sep 3, 2022

Conversation

CapnBry
Copy link
Member

@CapnBry CapnBry commented Aug 16, 2022

Changes behavior of the TX module when a CRSF connection is established, to not transmit any data until either a COMMAND_MODEL_SELECT_ID command comes in, or it has paused at least the duration of an RX timeout for the current rate. The purpose is to prevent changing the switch mode mid-stream on startup.

Problem

Reported on Discord by user Goddo.

  • Set up a model in the handset with ReceiverID 0, and configure the TX module for FullRes 333Hz, 8ch mode, Std Telemetry.
  • Set up a second model in the handset with ReceiverID 2, configure the TX module with all the same parameters except 16ch mode.
  • Using the ReceiverID 2 model, connect an RX
  • Power down the handset
  • Power up the handset
  • Observe that the servos are going nuts on the receiver as it is in 8ch mode and the TX is sending 16ch mode

This is a major issue because, as Goddo points out, users may reboot their handset if they think there's something wrong with the connection and this will result in an uncontrollable model as CH1-8 become CH9-16 with every other packet.

Cause

When the TX powers up, it has Model 0 selected. The handset sends a channels packet, which triggers us to go from noCrossfire -> disconnected state and start sending packets. The RX immediately will go to tentative, since it is locked on the sync channel of the correct packet rate. Shortly after, we begin sending packets to the handset, which triggers it to send an updated RecveiverID. However, since the RX has already moved out of disconnected, no switch mode change is allowed and it stays locked on the original switch mode.

TX
CRSF UART Connected
hwTimer resume
set rate 5
hwTimer interval: 3003
Config LoRa Adjusted max packet size 48-48
5474: Started // leaving noCrossfire state to disconnected
5474: switch=0 // first SYNC packet sent to RX, as Model 0
5486: model=2 // got new ReceiverID from handset
5489: switch=1  // tries to SYNC packet with new switch mode, but too late, RX has already locked to switch=0

RX
Config LoRa tentative conn
19950: switch=0 // first SYNC packet
19974: switch=1 // new switch mode in SYNC packet 24ms later
got conn
New TLMrate 1:2
Timer locked

This is a subissue of #1647 I had pre-3.0 where we'd start TXing and then not actually change the value, but didn't fix the case where the RX had already locked to the original switch mode.

Resolution

Simply don't send any packets until there has been a chance for the handset to send us the ReceiverID. Instead of going straight to disconnected when a CRSF connection begins, go to awaitingModelId and wait for the ID to be sent. The timer is still started, so that opentx sync packets can be sent to the handset and the mixer can adjust while we're waiting.

There are two ways to exit this delay:

  • A COMMAND_MODEL_SELECT_ID is received from the handset, confirming the proper ReceiverID. In this instance, instead of just switching immediately, the TX will send syncspam at the current packet rate with the new packet rate and switchmode before switching. This institutes a proper mode switch if the ReceiverID is changed on the fly during an active connection.
  • The TX has stayed in awaitingModelId long enough for the receiver to timeout and disconnect at the current packet rate. Currently all packet modes have this set to 5 seconds. This is to hopefully cause a receiver to allow a new switchmode if it still had a lingering connection, such as if the TX module was jostled and lost power and reverts to ReceiverID 0.

Existing Bug

This also includes an adjustment to the config loader. Without this PR, every time the TX starts up, the MODEL_CHANGED flag is set, which means a config commit has to happen on every boot. The other changes in this PR exposed this along with a timing issue which caused us to miss the ModelID set command on startup if we weren't transmitting immediately. It clears the "modified" flag on config after SetDefaults is called. I am like 85% sure this is ok, but I stared at it for an hour and I am still 85% sure it is ok. It is definitely ok for STM32 so that makes me 185% sure it is ok?

Breaking Behavior

Handset firmwares which do not send a ReceiverID will now experience a 5 second delay before they begin transmitting.

For Discussion

Currently the RX will never switch if it receives a switchmode change because of the suggestion "Well what if the packet is sync corrupted and has the wrong switch mode?!" However, if the switch mode has actually changed, then this leaves the RX in the unique position that it will never process the data properly. Perhaps for future work we could institute this procedure:

  • If connected and switchmode changed in sync packet, stop processing RCDATA packets and stop sending telemetry. This prevents new possibly wrong updates from being used, and not sending telemetry might speed the TX going to disconnected and sending more sync packets.
  • Wait for the next sync packet. If the switchmode matches what was in the previous sync packet, perform the change and resume normal operation.

That's a pretty big change so I'd want to do that in 3.1 rather than try to make a second large behavior change when release is pending.

@CapnBry CapnBry added bug 🐛 Something isn't working V3.0 🚀 labels Aug 16, 2022
@CapnBry
Copy link
Member Author

CapnBry commented Aug 16, 2022

Jeebus how are all my PRs so long?

src/src/tx_main.cpp Outdated Show resolved Hide resolved
@CapnBry
Copy link
Member Author

CapnBry commented Aug 19, 2022

Tagging this Do Not Merge for now as the original reporter says no change in behavior.

@CapnBry
Copy link
Member Author

CapnBry commented Aug 24, 2022

Removed Do Not Merge tag, reporting user has reported all working as expected now after e9010d6

Copy link
Collaborator

@pkendall64 pkendall64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find on the config problem.

@AlessandroAU AlessandroAU merged commit d3e75ac into ExpressLRS:3.x.x-maintenance Sep 3, 2022
@CapnBry CapnBry deleted the tx-tentative branch October 25, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working V3.0 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants