-
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
[WIP] initial bidirectional dshot support #20749
Conversation
@@ -1,6 +1,6 @@ | |||
/**************************************************************************** | |||
* | |||
* Copyright (C) 2012, 2017 PX4 Development Team. All rights reserved. | |||
* Copyright (C) 2012, 2017, 2022 PX4 Development Team. All rights reserved. |
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.
* Copyright (C) 2012, 2017, 2022 PX4 Development Team. All rights reserved. | |
* Copyright (C) 2012-2022 PX4 Development Team. All rights reserved. |
esc_status_s &esc_status = _esc_status_pub.get(); | ||
esc_status = {}; | ||
_esc_status_pub.update(); | ||
up_dshot_set_erpm_callback(&DShot::erpm_trampoline, this); |
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.
TODO: review logging and publication
I think it should be sufficient to let the lower level dshot driver capture (and store) the eRPM feedback (with timestamp) and leave it to the dshot module to handle populating esc_status
and publishing. We could schedule the work item to run sooner if desired.
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.
Right, or we just publish every time right after we send out the new motor commands. That way, we won't have the very latest result but that shouldn't matter too much as the numbers come in with a delay anyway because we retrieve the results round-robin style.
Thanks @dagar, I'm planning to get back to this soon. |
e57526a
to
5849dcd
Compare
I'm picking this back up. I just rebased on top of main, and it still works, so that's a start. |
5849dcd
to
f31898e
Compare
@amrathod this won't work for the Pixracer. The current work is only for STM32H7 based flight controllers. |
@julianoes Any update on this feature merging into |
Still on my todo list yes. |
Hi, I just wanted to let you guys know that I built this pr on mro-pixracer pro and I can see the esc_info and status in mavlink console of QGC, however, the motors don't arm. I tried to see if the motor spins through QGC, but it doesn't respond. |
f88424b
to
0ad65b8
Compare
@dagar I've just rebased this and added a param to explicitly enable bidirectional dshot. I think the next step is to figure out how to generalize the timer configuration for boards other than 6X or come up with a way to define the dshot timer and channel defines in the board config. |
@julianoes thanks so much for all your work on this. Have you been able to ascertain how fast the erpm is publishing for each motor? |
So it's reading the erpm feedback round robin style, so if you send out dshot at 800 Hz, you would get feedback at 200 Hz for a quad, or 100 Hz for an octo. However, I need to do a bit more testing and verification to check if that's working like it should. |
e7171cb
to
647bd84
Compare
@spencerfolk I had a look at how much of the feedback we fail to parse. In the current state on the bench at 800 Hz gyro rate I get these numbers:
This means about 10% of the measurements read invalid/zero. |
I added the conversion from the period sent via dshot to RPM based on the pole count set via parameter, and this is starting to look usable. For my bench setup I get 34000 RPM which might be about right given my motors are 2300kv * 15.2v = 34960. There are still some spikes at high RPM that I don't understand though. Note that the data below was captured with these topics increased in logger:
|
On a ModalAI FCv1 with STM32 F7 the bidirectional dshot PR has a hardfault once the bidirectional param is enabled. Prior to it enabling it seemed OK, but dshot outputs do not work. Previous changes worked on |
Oh, right, I don't think it would work on F7 in its current state. Only H7.
Interesting. I guess I can try to find out using another F7. |
Thank You @julianoes! I recently tested the bidirectional feedback on a pixracer pro, and verified the readings with a digital tachometer and the readings were close. I will post a log file within the week. |
I am not familiar with dshot protocol, but do you foresee any changes that will be required in the Currently working on bidirectional control for uavcan escs, such as for VESC, where their range is from -8191 to 8191 -> (max throtte in negative direction, max throttle in positive direction), but so far it seems I can only get this working by adjusting the mixer library. In BeliHeli, we could set the zero throttle position to 1500 PWM, which allowed us to control the motor like a servo in v1.12. This discussion should live in a different PR, but just mentioning here for now. Planning on PR those changes soon. |
Thanks for the note @henrykotze. I assume the mixer overall might need some changes to support both directions but I'm not 100% sure. Regarding bidirectional support and negative direction, I don't think it matters for the eRPM reported which is actually the period reported, and I think this period might always be positive for both directions. |
@julianoes Hello, thanks for your contribution. I try to use bi-directional dshot, when I use your original code, it works. But when I try to add some of my own code in the firmware (Mainly publish the I removed the callback function |
This still includes a couple of hacks and debug statements.
Signed-off-by: Julian Oes <[email protected]>
This adds the param DSHOT_BIDIR_EN to enable bidirectional dshot. Signed-off-by: Julian Oes <[email protected]>
This could potentially enable other H7 based platforms. Signed-off-by: Julian Oes <[email protected]>
Otherwise we publish a lot of duplicate data for little gain. Signed-off-by: Julian Oes <[email protected]>
This calculates the actual RPM from the period transmitted via dshot to based on the pole count. Signed-off-by: Julian Oes <[email protected]>
c1fad98
to
7eb919a
Compare
Rebased and force-pushed. Still working on my bench. |
Hi, based on the PR, I got bidirectional DShot running on an F4 (MambaF405MK2) with four channels in parallel (no round-robin needed). I did quite some heavy lifting on the code, but maybe I can help at some point on making the code more adaptable to other boards. |
@FHermisch nice! Do you require 4 DMA channels for that, or just using one? Feel free to make a PR with your changes against this branch so we can have a look at it. |
Somehow shifting the timer enable around made a big difference and got rid of the spikes. Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
@julianoes I use 2 DMA streams (DMA Request "UP") for writing out DSHOT data to two channels each in burst mode. Then I switch to using 4 DMA streams (DMA Request "CAPTURE") for reading-in the BiDi data. The code of the PR was running to slow on the F405 to be able to switch in time and be ready to capture the BiDi data, so I had to do low-level pushing of registers into the timers and the DMAs to be fast enough - sadly no code which can be directly integrated at the moment.
What helped me a lot:
I will compare the PR-Code with my code again and see if I find parts for which my solution will immediately help to find a proper generic solution for everyone. |
I just tested the Dshot code on px4_fmu-v6c and found that the data shows many spikes. I checked the code and found that the spikes are mainly caused by failed reads. In case a read fails, e.g. due to a CRC error, the code returns zero. I changed the code to make it return the last read value instead, which resulted in the following data: In addition to this, I found that this code can cause QGC to get stuck on the FC startup time after the user changes FC parameters. This seems to be because QGC is checking some FC status that is failing, which is causing QGC to get stuck. I'm not sure if this is caused by the Dshot code or an upstream problem. |
I'm interested in this feature too, thank you @julianoes for working on this! |
Has anyone successfully tested it on the holybro 6X flight controller? I tried multiple versions, but still failed. It always gives the following error: Motor output/control error. |
That's what I used to develop this. |
I tried it on two new holybro 6X flight controllers and a new holybro 6C flight controller, and the same problem always occurs. |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/trying-get-data-about-the-motor/36530/9 |
Have you solved this problem? |
#23863 replaces |
Opening a pull request with @julianoes initial work to increase visibility (a lot of people are interested in this). #19861
TODO: