-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fmuv6xrt implement bidirectional dshot #22583
Conversation
We are using PX4 and a fmuv6xrt to interface a new acoustic vector sensor on a multicopter with dshot ESCs. Thank you @PetervdPerk-NXP for coincidentally working on this PR in the same timeframe that we can use this. Our module runs as a task, so a uORB subscription callback would be an ideal interfacing mechanism. The eRPM values will be used as a component of a spatial filter to remove as much drone egonoise as possible. Ideally, published RPM values are available at the same rate as the motor update interval, but with much of our processing in the frequency domain, buffering and reasonable latencies are fine. We will certainly end up averaging/filtering the eRPM values. Hope this helps. |
0816d09
to
f00b6dc
Compare
Depends on PX4/NuttX#297 |
Either way works for me. We can initially just store the result in the callback, then publish on the next motor update iteration to uORB. |
Modified to work in conjunction to work with UART based telemetry
Needs testing @bkueng are you be able test this on a real drone or @dk7xe could you help out here? |
565839f
to
f337e56
Compare
If i get access to bidirectional Dshot ESC, yes. |
I currently don't have a setup |
Yesterday I cherry-picked the three commits into my fmu6xrt build based on PX4 v14.2, and tried this feature out on a new quadcopter build with four dshot 1200 ESCs. Prior to the update, I was able to read the RPM, voltage, and current over the 3rd telemetry wire, using the ESC_STATUS uORB message, but not after, and also there is nothing being streamed out TELEM1. Before the update, I captured this info from ESC #1:
After the update, I was not able to get a response either from ESC_STATUS, or dshot esc_info. When DSHOT_BIDIR_EN is false, the system will ARM and spin the propellers, but with no telemetry. When DSHOT_BIDIR_EN is true, I get ESC 156 offline errors for all motors during ARM, even though the parameter COM_ARM_CHK_ESCS is disabled. Note that even with DSHOT_BIDIR_EN true, I am able to manually spin the propellers using Actuator Setup. They just won't ARM. Appended below are the dmesg logs obtained with the new code, for two cases: DSHOT_BIDIR_EN true and false. We could use this feature ASAP, I'm happy to help move this along. Please suggest what debugging logs/messages/strategies would assist in this goal. dmesg (DSHOT_BIDIR_EN false):
dmesg (DSHOT_BIDIR_EN on, ARM not permitted):
|
Hi @jwwaite Thanks a lot for testing, I see you're using a TEKKO32_F4. Looking at the output I see CRC errors, are you running BDSHOT on 1200? Does lowering to 600 or 300 improve it? Overall breaking telemetry with normal DSHOT is not good, I would have to look further into that. Maybe this merging BDSHOT with TELEM goes wrong. |
Hi @PetervdPerk-NXP, I will drop the DSHOT speed and look at the results. With respect to "3rd wire" vs bi-directional telemetry, aren't both methods designed to coexist? If so, I should be able to read the third wire into TELEM1 (which also has voltage and current), while TELEM2 outputs RPM at a higher rate? Jim |
Thanks I've been testing with Aikon AK32 35A 2-6S ESC running BLHeli_32 that unfortunely doesn't have a telemetry wire. I'll see if I can get other ESC's with BDSHOT and telemetry to improve my test coverage.
We should check the BLHeli_32 documentation to determine if that's possible. Btw there are even some ESC's that can output voltage and current over BDSHOT using Extended DShot Telemetry (EDT) but haven't acquired ESC's that run the correct version of BLHeli_32 to support that either. |
Dropping the DSHOT rate to 600 eliminated all the CRC errors. This quad is a fairly large 600 mm frame, and all the ESC wires are unshielded twisted pair. No problem running at the lower speed if necessary. If we can get RPM data at half that rate, we're good. There still isn't any telemetry information presented in the dshot app, or on the serial line. In my setup, TELEM1 reads the 3rd wire telemetry info, which works correctly in the pre-PR dshot code (RPM, voltage, and current all present in the ESC_STATUS message, but at a slow rate with latency), but is missing in the PR code. I've set TELEM2 as the output port in the new code, and nothing is there either. I've seen that blog post on DSHOT before. In fact, it seems to be the closest thing to a specification that exists online. Note, that post specifically mentions that the 3rd dedicated back-channel wire carrying telemetry information is not really part of the DSHOT protocol. Thus, I would expect that ESCs that implement the first method that also support bi-directional DSHOT could do both in parallel. But you're right, we should check. In any event, if present, the dshot driver should allow 3rd wire data to pass through. |
I need to put the dshot RPM data out over Cyphal-CAN, not read it from a UART. So, I'm not sure why UART telemetry is even necessary for bi-directional dshot. Why not just leave the existing 3rd wire telemetry code in place, including the lower rate esc_status pub, and add a new dshot telemetry message and pub at a higher rate? Initially this could be just the RPM, but for ESCs that support EDT there could be additional data. |
Merging of data come from a dev call discussion with @dagar. Idea is to support all 3 telemetry methods
I hope to receive my ESC with a telemetry next week to see why this PR breaks case 1 and 3. |
As far as i know bidirectional dshot was implemented because Betaflight is using RPM for control loop. To do not block the dataline that long and to stay in sync just eRPM is reported back. All other telemetry values are still reported via the normal telemetry line. This is what ESC's with bidirectional have implemented nowadays. The extension to more telemetry values over dshot is quite new and not yet implemented widely. |
72ebccc
to
7d9ab35
Compare
@jwwaite I've found the culprit and got it work with bdshot to work with UART telemetry simultaneously. If you could test it and verify it's working with your setup then it's ready for review.
Also added a CRC error counter for UART telemetry.
|
7d9ab35
to
eb57942
Compare
I tried the new code (once again, cherry-picked in), but moving the system to ARM results in an ESC failure message. Shortly thereafter, the Nuttx reboots, and this occurs cyclically about every 5 to 10 seconds. I was not able to get the motors to spin, and so wasn't ever able to record a valid RPM. When BIDIR dshot is disabled, the reboots still persist. However, whether or not BIDIR is enabled I can use QGC Setup to manually spin the propellers, but that mode does not allow access to the ESC_STATUS message.
Since there are still CRC errors, I dropped the rate to dshot 300. The reboots still occurred. I should mention that my 14.2 PX4 branch https://github.com/AIVS-Inc/PX4-Autopilot/tree/ares_dshot_new is 41 commits behind main, although your commits (not yet pushed) merged with no problem. I tried updating to the latest main this morning but Cyphal wasn't working on my branch. That is necessary for my system. I'll look into that issue next week, after a long weekend (back Tuesday). In the meantime, if you have some suggestions on how I can provide better debug information, let me know. I have a scope here, for example. |
Unfortunately I can't access the repo you're linking, I could try your build tomorrow if I can acces it. For testing I recommend getting a Pixhawk debug adapter using the build-in usb-uart you may be able to see see why it's crashing. Regarding the CRC errors if it's stable I think it's manageable.
You could play with the bdshot_tcmp variable by changing-1 to -2 or 0 this calculates the baudrate of the telemetry unfortunately dshot isn't really exact with baudrate specifications so there's a big margin here.Overall scoping the signal and measuring bit times would also be interesting. |
I'm not into failsafe either maybe @dagar could help here. What happens if you disable 3rd wire telemetry and only run bdshot? |
I got it to work by disabling another ESC related parameter: FD_ESCS_EN. Both erpm and wired telemetry are reporting (using ESC_STATUS and dshot status to confirm). There are still a quite large number of CRC errors, for all motors, regardless of the DSHOT speed. I'll look at the effect of those counter values again. |
I confirm that: I can use this now. Is there anything else you would like me to test regarding this feature? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PetervdPerk-NXP Nice work.
@julianoes do we want to bring in your STM32 dshot PR changes with this one? I am happy to test the STM32 portion. |
The API additions are in You would have to implement
I would rather just merge this PR as is, before it really becomes a huge one and go phased changes for other hardware/features, i.e. even adding Extended DShot Telemetry (EDT) as well. |
ebe9f53
to
8db695f
Compare
Hey @bkueng can you help review if we ship you a sample board? |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: |
8db695f
to
de86ac5
Compare
a77babf
to
61360fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now.
Can you do another rebase and ensure no targets exceed the flash limit?
channel != telemetry_index, we've to count from 0 and increment for each enabled ESC
61360fc
to
4ca8b8f
Compare
@bkueng rebased CI looks happy now, Thanks for reviewing. |
Partial continuation of #20749 but then on fmuv6xrt hardware.
So I took over the
DSHOT_BIDIR_EN
and init sequencesThe low level dshot driver fully support bidirectional dshot on 8 channels.
In the current pr you can read out it through
dshot status
But also get interleaved with the
esc_status
publisher allowing co-existence with dshot telemetry