olliw42
Reset mavlink parser when a radio frame is lost to avoid likely loss of valid message(s) in the next frame(s)
When a frame is lost, the parser may be in the middle of parsing a message. We should reset it to start looking for the next frame so it doesn't discard all or part of the next message(s) while trying to finish accumulating the damaged message.
@olliw42 , you didn't say yet what you concluded after our call. Perhaps you are still thinking about it. But, I'm submitting this here anyway for further discussion.
The high frame loss of the current R9M seems at first a good environment for test, but the loss is so inconsistent that it's hard to judge the improvement. When I averaged the time required to download parameters for 12 runs with and 12 runs without this change, the average showed a little less than a second improvement, so, at least this code didn't make it worse.
In any case, this needs a good code review before possible merge.
@brad112358
first off, MANY thx for doing this, much appreciated
I could only very briefly look at it, but I think this can't be right. I would have expected the api to a reset call to appear before the serial putc please remind that we can have diversity also, so it must be inked into the code at high enough level I might be totally wrong, as said I just could look very briefly at it
it's hard to judge the improvement. When I averaged the time required to download parameters for 12 runs with and 12 runs without this change, the average showed a little less than a second improvement
well, I might have indicated that you may overestimate the benefit effect
this needs a good code review before possible merge.
not only this, it needs - IMHO - especially a good experimental validation (and this is the part I found difficult for the retransmission). Th eonly way of doing this I see currently (which doesn't mean it's the only way, hopefully not) is to create a detailed debug output and analyse very carefully for each transmission/reception if things happened as expected ... very tedious
you didn't say yet what you concluded after our call. Perhaps you are still thinking about it.
LOL, I simply had other things on my mind. What seems clear however that I can't be negative.
many thx again
@olliw42 Yes, the diversity code is still not clear to me so I may well have gotten details wrong. And, unfortunately, I don't have diversity hardware to test with. I don't understand what you mean by your sentence that ends in "can't be negative".
I just took another look at the code. I think I got things in the correct place for diversity, but I noticed something else I missed. I'll try to re-test and push an update tomorrow.
you may be correct that it's in the ccorrect function but I can tell you I don't like how it's spaghettizied, I think on logical grounds that it should be inbetween lines 380-381, that's there the place is to go IMHO I also don't like the naming, but I guess I can correct that lateron
I guess my first approach would be to just have in said location if (previous_frame_missed) sx.handle_previous_frame_missed(), and everything else would go I guess
there should be a similarity (equality) for the logic of detecting missed frames with what is needed for retransmission. So, please do the detection of the previously missed frame with the extension of retransmissions in mind. Otherwise one would spend ages to validate this PR to just throw it away and having to spend all the work to validate again.
when you call the reset in handle_recieve_none() the connect state may not be yet determined correctly, ergo race condition, ergo possible source of issues
even worse, we may also miss a cmd, in which case it would be incorrect and terrible to reset so, you want to reset only if the missed frame indeed was a frame with payload ... however there is no way to know that if a frame was missed ...
we probably would need more header bits to just do that reset thing properly
Which file are you referring to lines 380-381 in? When does cmd frames get sent? Only when configuring Rx? Yes, I agree we need to consider a future retransmission implementation when examining this PR.
Which file are you referring to lines 380-381 in?
ah, sorry LOL. mlrs-rx.cpp https://github.com/olliw42/mLRS/blob/main/mLRS/CommonRx/mlrs-rx.cpp#L380-L381
When does cmd frames get sent? Only when configuring Rx?
so far, only on connection and when configuring Rx
I am not sure if there ever could be another scenario, in some sense I would hope not
and then there is of course the bind state, which is a totally beast however, and it is non recoverable (i.e., needs chip reset to resume normal operation), so no issue
@brad112358
alternative implementation here https://github.com/olliw42/mLRS/commit/542a4a66decbd59d3e89b58978d7e59106345337
I concluded that it's sufficient to do the loss frame trigger in the "high-level" part of the code, which makes the implementation so simple and clean. MavlinkX ensures with very high probability that the start of a mavlink frame is detected.
Needs more testing and validation of course before it could go main.
the mavlinkX approach to the problem has been PR'ed now: https://github.com/olliw42/mLRS/pull/93
Let's close this. Depending on the where mavlinkX goes with respect to features, latency, etc, we can continue discussion later if it seems worthwhile to solve the framing recovery problem without mavlinkX.