search

ZZ-Cat / CRSFforArduino

An Arduino Library for communicating with ExpressLRS and TBS Crossfire receivers.
GNU Affero General Public License v3.0
129 stars 21 forks source link

CRSF frame parsing timeout repeatedly expires thus dropping all frames #108

Open britannio opened 4 months ago

britannio commented 4 months ago

Is there an existing issue for this bug?

What development environment are you using?

PlatformIO

What board are you using?

Raspberry Pi Pico RP2040

What part of CRSF for Arduino is this bug related to?

RC Channels

Current behaviour

When I set up the library to get rc channel data, the callback will run but the channel data never changes. I only encounter this issue when my Pi Pico is connected to the Adafruit PiCowbell CAN Bus.

Expected behaviour

The rc data should update as normal.

Steps to reproduce

I'm unsure how to reproduce this issue without the mentioned hardware.

Additional information

Removing the following lines fixes the issue. https://github.com/ZZ-Cat/CRSFforArduino/blob/26952d5351165cd8d6a42ac5d72a14b1c7eda686/src/SerialReceiver/CRSF/CRSF.cpp#L78-L87 So the frame expires before it is fully parsed, causing framePosition to be reset to 0 mid-way through parsing a frame and thus the CRC check will fail.

britannio commented 4 months ago

While I'm here, is it necessary to run the rc channels callback upon parsing a frame with a different frame type such as a telemetry frame? Likewise for link statistics. https://github.com/ZZ-Cat/CRSFforArduino/blob/Main-Trunk/src/SerialReceiver/SerialReceiver.cpp#L297

britannio commented 4 months ago

https://github.com/ZZ-Cat/CRSFforArduino/blob/26952d5351165cd8d6a42ac5d72a14b1c7eda686/src/SerialReceiver/CRSF/CRSF.cpp#L66

Increasing the timeout here may fix the issue but is the timeout necessary?

ZZ-Cat commented 4 months ago

...is the timeout necessary?

Oh gods, you have my intrusive thoughts in a nutshell, here. :laughing:

The time-out is calculated based on the number of possible size of CRSF packets (which is 64 bytes), the number of packets that are expected to be received, and the baud rate.
From what I understand of the protocol, a timeout of approximately 1500 µS is required to receive all bytes before the buffer is reset.
If an incomplete packet is received, it is assumed that the connection between your receiver and your target (EG development board) is either janky or broken... at least this is the theory behind it.

Removing the time-out altogether may fix your Issue. However, without the timeout, CRSF for Arduino will have no way of knowing the time difference between each byte received. This could create more issues than what your issue is trying to solve, here. It's more likely that my implementation of the time-out could do with a re-factor, as it was derived from Betaflight's implementation, and it's more likely that calling it in the context of the main loop() could be coming back to bite me here, because Betaflight's timeout is executed within interrupt context.

Increasing the timeout here may fix the issue but is the timeout necessary?

If you expand that function, you will see the calculation I am using:

timePerFrame = ((1000000 * packetCount) / (baudRate / (CRSF_FRAME_SIZE_MAX - 1)))

Punch that into your calculator with packetCount at 10 and baudRate of 420000, this yields 1500 µS exactly.
Full disclosure, I have no idea where the Betaflight devs even got 1500 µS from, because it makes zero sense.
You can increase the timePerFrame by passing in a packetCount higher than 10 when you use setFrameTime().

If you set the packetCount to 12, it will set the timePerFrame to 1800 µS. This may work for you.

While I'm here, is it necessary to run the rc channels callback upon parsing a frame with a different frame type such as a telemetry frame? Likewise for link statistics.

What do you mean by this? Could you elaborate on this a little more?

britannio commented 4 months ago

What do you mean by this? Could you elaborate on this a little more?

Each invocation of CRSF::receiveFrames() parses a single frame. The frame has a frame type and the ones accepted by the library are:

If the received frame is RC_CHANNELS_PACKED then the callback set in CRSFforArduino::setRcChannelsCallback should be invoked. Likewise, if the frame is LINK_STATISTICS then the CRSFforArduino::setLinkStatisticsCallback callback should be invoked. But right now the callbacks are being invoked irrespective of the frame type or whether the CRC check for the current frame passes.

If this change is made, the failsafe flag may need to be moved as it only relies on link statistics data.

ZZ-Cat commented 4 months ago

Each invocation of CRSF::receiveFrames() parses a single frame. The frame has a frame type and the ones accepted by the library are:

* `CRSF_FRAMETYPE_RC_CHANNELS_PACKED`

* `CRSF_FRAMETYPE_LINK_STATISTICS`

If the received frame is RC_CHANNELS_PACKED then the callback set in CRSFforArduino::setRcChannelsCallback should be invoked. Likewise, if the frame is LINK_STATISTICS then the CRSFforArduino::setLinkStatisticsCallback callback should be invoked. But right now the callbacks are being invoked irrespective of the frame type or whether the CRC check for the current frame passes.

Interesting. :thinking: Yea, nah. It shouldn't be doing that, and I will look into it. At this time, my focus is primarily with #103 (which has taken priority). So, it may be some time before I sort this issue out.