Add FLRC F-modes to ELRS SPI implementation #12936
Conversation
|
Do you want to test this code? You can flash it directly from Betaflight Configurator:
WARNING: It may be unstable. Use only for testing! |
CapnBry
changed the title
|
AUTOMERGE: (FAIL)
|
This is good reason to reject this feature request. |
CapnBry
force-pushed
the
elrs-fmodes
branch
2 times, most recently
from
337a2f9
to
c0df818
Compare
I had to refresh my memory on this because it has been a while since I worked with it and usually don't run F1000 myself. Running this code against master:
I must have been misremembering, or possibly there was some interaction with the version master from back in April, but it seems to be ok now, but I'd say it requires 2K PID loop or a 120MHz overclock to do 1000Hz. 500Hz always appears fine (and the current master does 500Hz LoRa as well so that's not changed). I've updated my original post. |
|
Ah yes, sorry about that. I believe I have fixed all the indention and opening bracket style issues now. EDIT: I tend to shy away from using force pushes normally, but as an "I'm a dumb developer" question is there another way to keep this one commit but not using commit amend + push force? |
Surely it can be documented? |
very unlikely as we recommend serial based ELRS to the manufacturers, with the at32 chips as well the lack of uarts is addresses vs the f411 |
|
Personally, not in favour of this. 1000Hz has marginal benefit because the resolution of feedforward is so poor at 1000Hz that the filtering delay means overall feedforward benefit is zero, and the improvement in P lag is not going to make any definite improvement to the flight control experience. If adding this made no hit to CPU that's OK, but if it does, that's bad. We would need to see a log of the scheduler time required for the ELRS tasks to figure out if this can get up. |
|
If elrs was to go to 11 or 12 bit on the other hand…. |
That's not really on the table here, SPI implementation or not, people are running Betaflight on 1000Hz ExpressLRS. I do agree with your expertise in the matter, but rejecting this strictly on the basis of it being 1000Hz implies that Betaflight should also prevent an external 1000Hz packet rate as input too, does it not? 😁 Of course it takes more CPU time to receive twice as many packets as 500Hz, you don't need any scheduler logs to know that. Every feature takes CPU time but isn't excluded strictly on that metric though. This is on the user to decide what they like, as the MCU is capable of doing it. Does it take too much CPU time? The performance of the quad I tested with seemed unaffected, even with bidirectional d-shot and all the normal filters on (including dynamic gyro / dterm / notch). Note that this PR also includes F500, which is extremely low-latency compared to LoRa 500Hz and is beneficial on its own. The average theoretical latency of F500 is just 1.439ms from gimbal to SPI pulling the packet into Betaflight, compared to 2.557ms for LoRa 500Hz. I don't think you can argue against a 43% decrease in average latency for the same CPU time, even if both these values may very well be below any sort of measurable impact on pilot performance.
ELRS OTA 4 will change all FLRC to 12bit resolution, but that change alone will just be a few lines of this code being updated for the new packet format so it would be nice if we already had a know working stable FLRC implementation to build on rather than both adding FLRC and making packet format changes to support OTA 4. DisclaimerI am not being an ExpressLRS zealot demanding this be included into Betaflight, but am simply supplying the code in response to requests I have gotten from pilots to include these modes. I've also been anti-SPI right from the start, but its use is somewhat inescapable in some use cases and extremely convenient in others. I don't know if 1000Hz makes people better pilots, but that's not what is on the table here-- Betaflight already does not prevent 1000Hz RC input. Running 1000Hz SPI also does take more CPU time, but spare CPU cycles do nothing. A typical F1000 user would probably not be using other features like GPS, or a ton of OSD elements, so that synergy reduces the CPU time argument even further. |
|
i personally do not mind the option to exist, but maybe with disclaimer somewhere (should it be in configurator? 🤷♂️ -- such would need to alert to both CPU and FeedForward problems) two things come to mind as already presented
|
|
any movement on this? |
|
Happy model are hell bent on continuing to use spi elrs as well. They really need to stop using the f411 on every thing 🤦♂️ |
Oh I don't think that's going to happen. I don't even know how to profile these devices or have the software tools to do it effectively. Although actually (pushes glasses up) the FLRC implementation here is more SPI optimized than the LoRa version because there is hardware CRC for FLRC, and therefore bad packets are dropped early in the chain resulting in less SPI traffic than bad LoRa packets. Measurable efficiency! I kid though, that's just basic decent coding. 😅I refuse to turn this FLRC PR into a debate about 1000Hz Ideology and Usefulness! |
|
My views on 1000Hz are well known. When ELRS goes 12 bit, rather than the current 10 bit, and the incoming signal from the gimbal sensor clean enough, and the input resolution of the radio AD converter is good enough, and the data path in the radio is jitter free, that's when 1000Hz will be more useful than it is now. Once we get 4.5 RC1 out, if I get some ELRS SPI boards, then I can look closely at optimising feedforward and providing sensible values in our Presets. So for sure, let's set debate about 1000Hz aside. The general topic of SPI Rx is a difficult one. The FrSky SPI Rx implementation was very prone to random link failure and I don't think anyone knew why. This gave SPI Rx a bad reputation, generally. Well maintained, efficient SPI Rx code that works reliably is something that I would continue to support actively. It's latency is considerably less than serial. And on F411's it frees up a serial port, and that is helpful to some people. So let's be clear, I have no objection to SPI Rx code for ELRS, so long as it is as efficient as possible, well-maintained, works reliably, and that its reliability is validated as carefully as possible by the people maintaining the code. |
Suitable tools exist within Betaflight already, so you have all the tools you need. The basic things to check are flash space requirements and the tasks command in CLI:
Using the tasks menu requires invoking the command at say 10 or 15s intervals, repeatedly, to get a 'feel' for the typical numbers. Then flash the new code, and see if those numbers change significantly. If the new code reduces task time, and delays other tasks less often, that's really good; if it takes longer and causes other tasks to over-run, that's bad. More advanced testing involves checking execution time Code that runs only occasionally (eg only when new data arrives) has a 'resting' impact while it is 'idle', and an 'active' impact in the CPU cycle in which it is triggered. Some aspects of Rx code are, I think, spread over several cycles to 'smear' the cost per cycle over several cycles. I don't know if that is the case for the SPI code. For SPI Rx code, it would be good to know the CPU cost every CPU cycle; that way we know what it costs when it is 'idle' and that it costs when 'active'. Once we know that, we can make rational recommendations on loop time so that there are enough CPU cycles that these processes can run without messing with other bits of code. If the 'active' task is very, very high on CPU, then it could be split over two cycles. Without seeing data on CPU time cost, we won't know what's going on. To find out, we can set timers and log the time taken for parts of the code to execute. Check the We can also log the delta time between absolute start time of each iteration of the code, in microseconds, to check that the code executes at the expected time intervals, which may detect impending failure or significant over-run issues. Doing this is time consuming, but not difficult. Just use an existing debug with spare space, if you like. There is a debug When logging for these purposes, you'd put transient times into a static value so that even if logging at 1:2 you'll see that value from its last iteration. Disabling nearly all other elements in the log may permit logging at 1:1, and this is possible on many boards, and 1:2 is certainly achievable on F411's. That's enough to get a good reflection of the 'idle' period CPU cost. You can see examples of timing tests done this way in the Mag Pr True, it does take a bit of time to set up, but once set up, it is very easy to evaluate whether a particular part of the code runs more efficiently when coded differently, and to learn about the overall CPU cost of the code. The main concern with SPI code has been its impact on the CPU and the possibility that if the CPU gets excessively loaded, the link may fail. At present we are in a bit of a vacuum with no information to run with at all. If we continue to support SPI Rx then:
|
This comment was marked as outdated.
This comment was marked as outdated.
|
As a dedicated Tiny Whoop pilot, I feel obligated to contribute to this discussion. In my experience and observing trends at our local flying club, SPI-only boards are extensively used and will likely continue to be a mainstay in our community for some time. Unfortunately, these are not going away anytime soon. I would welcome the addition of the F500 mode this PR brings and look forward to the addition of 12-bit resolution. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Sorry put this on 4.6 to soon. Discussion ongoing. Please correct [me] if wrong as I do not intent to give false impressions. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Oh I am such a good developer I typed a whole pile of text, verified the markdown looked good in Preview, did not hit the post button 😣 Shorter summary: Thank you, ctzsnooze for your detailed explanation of the facilities available for profiling. I will likely revisit that message if I need to gather information. I did not extensively profile the code or attempt to make the PR bigger by refactoring / optimizing any of the ELRS SPI code. I also did not add FLRC D-modes as they require changes to the processing and would be a larger changeset. The only change this adds is in the one-time initialization of setting up the RF chip, to put it in FLRC vs LoRa mode. I then did A/B tests with master with the PR applied or without to check for any regressions. At 500Hz LoRa vs 500Hz FLRC the tasks showed similar CPU usage, and 1000Hz doubled that so everything checked out. The core ELRS SPI code is all the same, so the performance of one vs the other should be identical (at the same rate) except for the early bailout if the FLRC has a hardware CRC check failure. I've been flying F500 since May on my whoop (8k/2k) using this code and seen no issues. Moving forwardIt would be great if there was someone who was motivated to become the full time maintainer of this code, kept it up to date and would have good time optimizing it for size and speed. I, on the other hand, announced a year and a half ago that nobody should buy ELRS SPI because nobody on our team planned to support it. I assisted with the ELRS 3.0 update because it made me sad and embarrassed to think of pilots having to fly ELRS 2.4 forever 😅 This change was just (someone) "Can you make SPI do FLRC? 1000Hz?" me: "I dunno, sure, it is just changing some parameters". A couple days later I banged it out in a few hours. I greatly appreciate the time you and the rest of the team puts into creating and maintaining betaflight, and love how every year my quads just fly better. I only intended to make a small incremental improvement in the ELRS compatibility and not take on any sort of ongoing role. I would have much rather the ELRS SPI implementation never existed due to the user confusion, version compatibility and feature set issues, and workload it creates for both your and our team. |
|
It’s a little bit of sore point that the spi implementation on our side pushed our rc phase on 4.3 to 3-4 months and then elrs dropped support. It was a massive amount of work for something that was DOA. Happy model are also hell bent on continuing to use it and make new hardware with it also as a side note. Maybe they assist with with maintenance of it? |
|
@CapnBry I think we are both on the same wavelength, and I carry no sour grapes. ELRS has been massive for me and I'm a very strong supporter. I personally want to keep SPI ELRS as long as possible, and I do appreciate that there are a LOT of ELRS SPI users out there. One of the great virtues of the cloud build system, for users, is that the can include a defined code block when they flash the quad. And they can do this with release firmware. From the developer perspective, it's possible to compartmentalise the code. This helps us know who is responsible for maintaining it, and we can protect the rest of the code from defined code that isn't included by default. So it becomes a win-win situation. If, for example, all ERLS SPI code was put inside If this PR only improves ELRS 3.x support for ELRS_SPI users, then this approach is the way to go. If a user flashes it, and has problems, they can get back to you on this PR. It's even possible to use ifdef to handle 3.x vs 4.x support by putting all the existing code into Now I appreciate that this is a fair bit of work, but it is, I think, the right way forward. It works for me, and it should work for you and for the users. As to the timing of it, there is no explicit urgency if we go down this path. If we do not include this PR in 4.5, ELRS 3.x users get a 'known good' SPI solution in 4.5. I don't want to break that by merging this and discovering bugs in it. I'm sure you also accept that this conservative thinking has some merit. Its not clear how long to 4.6, but probably not as long as 4.5. So you could put the SPI code into Additionally, if ELRS 4.x comes out, you can do the same there, if supporting ELRS 4.x over SPI is what you end up wanting to do. I'm hoping this sounds reasonable. |
|
I've had no success getting the FLRC modes to work properly with this PR. When I attach a battery to my whoop, the F500 mode stops communicating shortly after. Looking at the receivers tab, it is visible that a few seconds after connecting the battery, the inputs suddenly stop. Other LORA modes function normally. I am using the TMOTOR 6A AIO and HM ES24TX ELRS version 3.2 Here is the tasks output prior to connecting a battery: and tasks output after connecting the battery |
|
I hope this video makes things clearer. You can see in the OSD that once the battery is connected, the LQ slowly drops to 0 |
That's really bizarre, but it seems like it loses sync then keeps losing sync more and more until it drops off. I'd bet if you changed to Wide switch mode (which doesn't allow desync) it would drop off instantly, or connect/disconnect over and over. I only have MPU6000-based FCs (Pancake and CrazyF4) and they do not show this issue so there's something different about that board when the battery is connected. BMI270 interacting with bidir DSHOT somehow in a way that blocks the RX process momentarily? Does it do the same thing if you start with your handset off, plug in the battery, then turn on the handset with F500? Really strange because the code all takes the same amount of time regardless of if the packet was received with LoRa vs FLRC so I can't even wrap my head around why it would work with one and not the other. |
Yeah I agree, the timing was just really not good. We were still quickly changing the OTA as we settled in to what our requirements and functionality needed to be when almost out of nowhere this implementation appears. The code quality is great, and in use it has proven to be stable so there's no complaints at all there. Like you say though, it just came a little early and being a completely different implementation of the ELRS protocol in a major project on a different release schedule, it created some real mayhem. It is possible finding a manufacturer to sponsor it might attract a developer to it, but I feel like somebody has to want to do it. There's another FC software that supports ELRS SPI (including FLRC) as well I do agree that we're all sort of scratching our head over who this burden falls to and I'm not exactly helping as I slowly try to back out of the room. I definitely don't think there's any rush on this code though, so as it gets figured out I have no problem at all pushing it back to 4.6 especially in light of JusticeFPV's report for which I have no real idea how to resolve. |
After a quick test, the receiver never connects when I turn on the radio after inserting the battery. The only difference I can think of is that the VTX is only powered when the battery is attached, which may have some relevance, as opposed to most AIOs, which power the onboard VTX when USB power is provided. I know he's a busy man, but maybe @SteveCEvans would have a better insight as to what might be happening here? |
|
DVR at bottom BETAFPVF4SX1280 ICM20689, on bench, TX maybe too close. 500hz (not D500), TX powered on F1000, TX powered on F1000, TX powered on trying again Flights DVR, stricty F1000:
|
|
Have any of the maintainers tried this on an SPRacingH7RF which has an H730 CPU running at 520Mhz and using memory mapped flash? @haslinghuis you have one right? |
|
Yes got a H7RF - but need a new ELRS module - waiting for upgrade [main] board for the TX16S. |
|
Done.
There's two lines different between supporting F1000 and not supporting F1000. I believe there is no reason to not include it.
Excellent observation, it is slightly slower. An ExpressLRS receiver must check every possible packet rate to wait for an incoming connection since each has different RF parameters and therefore can not receive e.g. a LoRa 500 packet while in FLRC 500 mode. Adding new rates increases the total cycle time. F1000 adds 160ms, F500 adds 320ms and these come first in the rate list so the LoRa modes experience 480ms longer connect times on initial connection. Reconnects after failsafe are not affected by this delay. |
Some racer using a pancake was sad his Betaflight didn't do F1000 so I added it, along with the other ExpressLRS F-modes. Note that it turns out the F411 doesn't have enough CPU power to do F1000+4K PID loop, at the very least have a slow updating OSD. For 1000Hz, a 120MHz overclock or 2K PID loop seems to be a necessity. Still, I believe this to be a good addition and if someone makes an SPI ELRS Betaflight FC that doesn't use a terribly overburdened MCU (F722?), it should work even better.
Note that I wrote this 3 months ago and haven't touched it since so I am a little foggy on the details. I have shared this back then with @phobos- who wrote the rest of the implementation and have heard 0 complaints 😋 as long as we go by the rule that "no news is good news" and not that no news means nobody has looked at it. So have a look at it!
This PR also removes unused CLI options to set
expresslrs_rate_indexandexpresslrs_switch_mode.I did not implement the ELRS D-modes, which would still be needed to bring the code up to full ExpressLRS 3.x OTA.