betaflight / betaflight

Open Source Flight Controller Firmware
GNU General Public License v3.0
8.06k stars 2.87k forks source link

Feature/dshot extended telemetry on unused ranges #11694

Closed damosvil closed 1 year ago

damosvil commented 1 year ago

This is code example on how to support extended telemetry on repeated and unused bidirectional DSHOT coding ranges. This code is backwards compatible to regular bidirectional DSHOT escs.

This pull request is related to issue 11677: https://github.com/betaflight/betaflight/issues/11677

You can see a demo here: https://www.youtube.com/watch?v=STiT7HIfz9E

And flight footage here: https://www.youtube.com/watch?v=CxIo7ElG3JA

KarateBrot commented 1 year ago

Can you explain what the goal of this PR is? What exactly are you doing and what are the results of this?

I don't get why we need high speed temperature readings for example... we can get it with normal ESC telemetry. Wouldn't it be better to only transmit RPM readings to make the Bidir Dshot data transfer as fast as possible?

It's very important that the RPM readings don't get compromised in transfer speed so that the RPM filter can react as fast as possible to RPM changes. If you can show/explain that the RPM readings still are as fast as before and don't get slowed down by other data, this seems like a really cool idea.

I guess that "repeated ranges" means that there is garbage transmitted anyways, so why not use it for useful data without compromising RPM data? Am I understanding this correctly?

limonspb commented 1 year ago

@joelucid could you please have a look at this idea too A more detailed explanation is here: https://github.com/betaflight/betaflight/issues/11677

damosvil commented 1 year ago

@KarateBrot The goal of this pr is to add missing telemetry data capabilities to Blheli_S based hardware such as temperature and even debugging data values.The idea for these additional telemetry data is to send them at 1Hz or less rate, so RPM telemetry is almost not interfered. Additional information like zero crossing or demag events can also be sent back to the FC this way, again at a slower pace than eRPM data.

Why do temperature readings are important? Because of the same reason than temperature indicator is important in a car. Maybe for the most of the life of the car we won't event realize that the temperature indicator exists, but when the motor fails the temperature indicator helps to diagnose the reason why the motor is failing. An example of this is a motor that doesn't spin freely. This motor can drain more current and for this reason the esc runs hotter. If a temperature indicator/warning is there we can realize that something is going wrong before we lose the kwad and everything it is carrying. We can land the kwad and repair it.

In addition to the previous reasoning if an esc temperature goes over the limit the esc firmware limits the esc throttle. This is completely unuseful in a kwad, because if one of the motors lose power and the FC ignores it, the FC cannot keep control of the kwad. Because of this reason limiting throttle because of temperature is high in an esc should be done in the FC.

I think that if the ESC sends temperature data once each second RPM readings performance won't be noticeably affected. Temperature doesn't usually rise so fast that it needs to be controlled so steadily, so sending it once each second is a reasonable rate.

For "repeated ranges" I mean that there are codification ranges that repeat the same coded values than a previous ranges, but at a lower resolution. I added an spreadsheet to issue 11677 where you can see those ranges in yellow. There are 7 ranges of 256 values that can be used for additional telemetry data: https://github.com/betaflight/betaflight/issues/11677

blckmn commented 1 year ago

AUTOMERGE: (FAIL)

joelucid commented 1 year ago

I kind of like the idea of providing low frequency telemetry other than rpm over the dshot channel! Fewer wires -> happiness.

That said the bidirectional dshot spec does not require a given implementation to use one particular coding for a given rpm telemetry value. So you can't know which values an implementation will use and making an assumption based on popular implementations changes the protocol.

One way to deal with that would be to have a new dshot command which queries the rpm telemetry protocol version and only enable the feature if the implementation supports it.

Another issue is that omitting the rpm telemetry send when you send low frequency telemetry data would delay the rpm telemetry packet. This could be worked around by only allowing a send if the encoded rpm hasn't changed but I'm not sure how often that would happen.

@sskaug what's your opinion?

joelucid commented 1 year ago

For completeness here's the coding ambiguity: the period is encoded in the form m << e, where e is 3 bits and m is 9 bits. For example the binary value 0000 1111 1110 could be coded as 1111 1110 << 0 or 0111 1111 << 1. jesc encodes by right shifting until bits >= 9 of m are 0. So if the period is >= binary 10 0000 0000 the encoding will have bit 8 of m set and e > 0. Which means that jesc does not use encodings which have e > 0 and bit 8 of m = 0. e has 7 non zero values (being 3 bits) and for each of these set bit 8 to 0 and you have 8 bits left which can carry information. That leaves 7 x 256 values which aren't used by jesc.

sskaug commented 1 year ago

This is a really cool idea. When bidirectional dshot was defined, I advocated to also add codes for temperature, voltage, current etc, but the interest at that point was low. If implementing this for BF, then temperature, voltage and current should all be added. No need for additional wire -> happy users. Also I think a rate of every second is slow, at least 10x of that should be a minimal loss of rpm info. And then about 300ms latency for each of temp, volt, curr is already kind of sluggish. Maybe an alternative packet every 32ms would be acceptable without noticeable loss to rpm filtering.

joelucid commented 1 year ago

We'd need dshot commands to enable/disable this, too. Not sure its worth enabling subsets of telemetry data. To save dshot commands we could use a single command to request low frequency telemetry and the esc would need to respond with a certain telemetry value which would indicate support for the feature. Then the version command would not be required.

It might be cleaner to add a header to the telemetry response - but given the critical timing issues around switching lines etc it might be ok to use the currently ambiguous telemetry codings. It's a bit hacky.

damosvil commented 1 year ago

I think that one command to enable extended telemetry data could be easy to implement. All extended telemetry are identified by the first four bits (eeen) of the telemetry data (eeen nnnn nnnn), when eee > 0 and the first bit n is 0 (i.e. 0010 nnnn nnnn, 0100 nnnn nnnn, ... , 1110 nnnn nnnn), so in for example the last identifier (1110 nnnn nnnn) could be used to code a status/event identifier to notify the ESC having extended telemetry (1110 0000 0000).

That identified could be later used to code state and events (ssss eeee) in the prefered form. Events could be programmed to be sent by the ESC if any unexpected behabiour is detected, for example, and state could define normal state (0000), overtemperature (0001), overvoltage (0010), overcurrent (0011), etc.

At this time identifier 0010 (0010 nnnn nnnn) is used for temperature. If you like we could use 0100 (0100 nnnn nnnn) for voltage, and 0110 (0110 nnnn nnnn) for current.

@joelucid @sskaug Below I will try to keep a draft as updated as possible. Please, let me know anything you want to be there.

damosvil commented 1 year ago

Draft to keep track of proposals

I will try to keep this draft updated with all proposals you have. Please, if I am wrong with anything let me know. I will try to update this draft assap. TBD - Stands for to be defined

Extended telemetry enable / disable

Extended telemetry enable

Extended telemetry disable

Coding ranges

Telemetry frame: "eeem mmmm mmmm" where "*_eRPM coding value = m 2 ^ e_**" (or eRPM coding value = m << e)

eeem mmmm mmmm

Feature autodiscovery

The only mandatory frame is State/events frame. Betaflight can easily discover other featutes as soon as it receives temperature, current and voltage frames. This way it isn't neccesary to activate individual features.

Frame scheduling

By default eRPM telemetry is sent back to the FC, and every ESC firmware will decide the best frame scheduling based on their configurations, use cases or preferences. So they will be defined as example, instead close defined.

Quick-Flash commented 1 year ago

"0010 mmmm mmmm - Temperature Celsius [-128, ..., 127]" Some esc's can get hotter than 127 degrees C, so I'd rather see a 0-256 range than a -128 to 127 C range. I very much doubt we will have esc's that will ever read below 0 C, especially while in use.

damosvil commented 1 year ago

@Quick-Flash I agree, but there are some people that fly on cold places, what about to code between -20 (0 decimal) and 234 degrees celsius (255 decimal)? I would allow some negative temperatures

sskaug commented 1 year ago

BLHeli_32 implements the KISS compatible telemetry, where temperature is encoded as an unsigned byte. So negative temperatures are clipped to zero.

damosvil commented 1 year ago

@sskaug, @Quick-Flash Ok. I update the draft with that range, and I will check how Blheli_32 & KISS codify current & voltage to update those ranges too

damosvil commented 1 year ago

Looking at kiss telemetry protocol I am seeing that they are sending temperature in one byte, and voltage, current and current consumption in 2 bytes each. I would prefer to send some of these in one byte or not sending some of them, otherwise they have to be splitted in 2 frames and won't be room for status/event frame. Please, I would need more experienced feedback here. @Quick-Flash, @sskaug, @joelucid, others

My proposal at this point is: 0100 mmmm mmmm - Voltage -> 0-63,75V, step 0,25V 0110 mmmm mmmm - Current -> 0-127,5A, step 0,5A 1000 mmmm mmmm - Current consumption hi - Like KISS 1010 mmmm mmmm - Current consumption lo - Like KISS

I would like to keep Status/Event frame to respond to extended telemetry activation/deactivation, and to be able to send diagnostic events related to ESC/motors. The goal is to know about a deffective ESC/motor before having problems during a flight.

KISS telemetry protocol

KarateBrot commented 1 year ago

@damosvil Just make sure we can get peak performance. Otherwise this PR wouldn't make sense. If that means breaking compatibility with KISS, then so be it. KISS would actually profit from your work as well in that case.

sugaarK commented 1 year ago

Why are we talking about current when all of the esc’s that sent current Over telemetry stopped been made about 2 years ago?

damosvil commented 1 year ago

Why are we talking about current when all of the esc’s that sent current Over telemetry stopped been made about 2 years ago?

@sugaarK To be able to diagnose every ESC separatelly and being aware of a faulty ESC/motor.

damosvil commented 1 year ago

@damosvil Just make sure we can get peak performance. Otherwise this PR wouldn't make sense. If that means breaking compatibility with KISS, then so be it. KISS would actually profit from your work as well in that case.

Ok, then I will update the draft with the following annotations:

0100 mmmm mmmm - Voltage -> 0-63,75V, step 0,25V 0110 mmmm mmmm - Current -> 0-255A, step 1A

No current consumption because it can be calculated by the FC's ADC integrating readings from the ESC 4in1 current sensor and in individual ESCs by integrating current telemetry readings over time.

I would also like to leave frame scheduling open so Blheli_32, KISS, JESC and others can decide the best scheduling for them based on their needs.

Except for the telemetry events extended telemetry is almost defined. Please, leave comments anything you don't agree on

sugaarK commented 1 year ago

Why are we talking about current when all of the esc’s that sent current Over telemetry stopped been made about 2 years ago?

@sugaarK To be able to diagnose every ESC separatelly and being aware of a faulty ESC/motor.

You can’t get individual esc current data unless it’s has individual current shunts for each esc and as I mentioned.. these died around 2 years ago..

damosvil commented 1 year ago

You can’t get individual esc current data unless it’s has individual current shunts for each esc and as I mentioned.. these died around 2 years ago..

@sugaarK Ok, I will remove current telemetry from the draft. I checked it. Sorry

sskaug commented 1 year ago

@damosvil, great that you are keeping track of a spec. We have started coding an implementation.

I strongly disagree to remove current.

Just for a statistics glimpse - out of the 25 last BLHeli_32 codes that we have distributed, 10 have current sensing/limiting capability. So a significant portion of the hardware out there is still delivered with (individual motor) current sensors. And as far as I can see, there are still many available codes, so using one for current most likely will not limit future extensions.

We are targeting one of temp/volt/curr every 32ms, rotating between them, so one datapoint for each about every 100ms.

At the moment we only intend to send state/event frames upon DIGITAL_CMD_EXTENDED_TELEMETRY_ENABLE/DISABLE, and we plan to send such ack frames 10 times.

damosvil commented 1 year ago

@sskaug If people are demanding firmwares with current telemetry then I will add current again to the draft. I think that 0-255A, step 1A, coding is reasonable, but please let me know whether I am wrong. Thanks for your comments.

This week I will be finishing my new testing kwad, so I hope I will be able to start coding this spec at the end of the next week.

sskaug commented 1 year ago

@damosvil - great that you will include current. A range from 0-255A should be the best choice for an available byte of data.

KarateBrot commented 1 year ago

Yeah it makes sense to include current. Even if some ESCs don't have individual current sensors available. The ones that have current sensing will still profit from it. It's also a reason for manufacturers who got rid of it to reintroduce it to their new ESCs.

sskaug commented 1 year ago

@damosvil We now have an initial implementation ready for BLHeli_32. Please let us know which ESC code (the name of the firmware as shown in BLHeliSuite32) you would like to have for testing.

damosvil commented 1 year ago

@sskaug The Blheli_32 ESCs I have are IFLIGHT_G071_4IN1 Rev. 32.7. Please, let me know if you need something else

sskaug commented 1 year ago

Testcode now posted here: https://github.com/bitdump/BLHeli/tree/master/BLHeli_32%20ARM/Dshot%20extended%20telemetry%20testcode

haslinghuis commented 1 year ago

Please fix

In file included from ../main/fc/core.c:45:
../main/drivers/dshot.h:112:55: error: unknown type name 'dshotTelemetryType_t'
uint32_t dshot_decode_telemetry_value(uint32_t value, dshotTelemetryType_t *type);
                                                      ^
1 error generated.
make[1]: *** [Makefile:774: ../../obj/test/arming_prevention_unittest/fc/core.c.o] Error 1
make[1]: Leaving directory '/home/vsts/work/1/s/src/test'
make: *** [Makefile:675: test-all] Error 2
##[error]Bash exited with code '2'.
damosvil commented 1 year ago

Cli basic testing commands for smoke testing: dshotprog 255 13 - enable extended dshot telemetry motor 255 1020 - start all motors at low speed dshot_telemetry_data - display all supported telemetry data received

2c9679f Custom Bluejay at https://github.com/damosvil/bluejay/tree/feature/dshot_extended_telemetry

Tested on a BETAFLIGHTF4 target with custom Bluejay. Arming sequence & cli work fine. OSD display temperature working.

Tested on a STM32F722 target with custom Blheli_32. Cli works fine. Arming sequence still not tested.

Tested on a STM32H743 target with custom Bluejay. Cli works fine. It doesn't arm, RPMFILTER message when arming.

Flying tests done, but all drones still not arming.

damosvil commented 1 year ago

@sskaug Working like a charm from cli on a Titan DC5. IFLIGHT_F722_TWING & IFLIGHT_G071_4IN1

Entering CLI Mode, type 'exit' to return, or 'help'

# dsho
dshot_telemetry_info    dshot_telemetry_data    dshotprog   
# dshotprog 255 13
Using all outputs.
Command Sent: 13

# motor 255 1020
Using all outputs.
all motors: 86

# dsh
dshot_telemetry_info    dshot_telemetry_data    dshotprog   
# dshot_t
dshot_telemetry_info    dshot_telemetry_data    
# dshot_telemetry_data
Motor1
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 585
    TEMP: 40degC
    VCC: 12.00V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0
Motor2
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 628
    TEMP: 41degC
    VCC: 12.25V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0
Motor3
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 585
    TEMP: 40degC
    VCC: 12.00V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0
Motor4
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 628
    TEMP: 39degC
    VCC: 12.00V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0

# dshot_telemetry_data
Motor1
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 600
    TEMP: 41degC
    VCC: 12.00V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0
Motor2
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 628
    TEMP: 41degC
    VCC: 12.25V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0
Motor3
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 585
    TEMP: 42degC
    VCC: 12.00V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0
Motor4
    TYPES: eRPM TEMP VCC CURR ST/EV 
    RPM: 614
    TEMP: 41degC
    VCC: 12.00V
    CURR: 0A
    ST/EV: 0
    DBG1: 0
    DBG2: 0
    DBG3: 0
sskaug commented 1 year ago

Cool, great to hear. Good job!

KarateBrot commented 1 year ago

@damosvil Looks great! One thing to note: Can you please get rid of all tab stops and replace each one with 4 whitespaces? It's not important to do it now, just a heads up that this needs to change before merging when it's ready.

The best thing would be to change the tab behavior in your code editor to automatically insert 4 whitespaces if you press tab, so that you cannot insert additional tab stops on accident.

damosvil commented 1 year ago

@damosvil Looks great! One thing to note: Can you please get rid of all tab stops and replace each one with 4 whitespaces? It's not important to do it now, just a heads up that this needs to change before merging when it's ready.

The best thing would be to change the tab behavior in your code editor to automatically insert 4 whitespaces if you press tab, so that you cannot insert additional tab stops on accident.

Ok, I will fix it in the following commits

damosvil commented 1 year ago

8356e6c

Pending:

Bluejay prerelease files can be downloaded here: https://github.com/damosvil/bluejay/releases/tag/extended_dshot_telemetry

Related flight footage (disable audio before watching): https://youtu.be/Hq_hNOZNzLM

@KarateBrot, @Quick-Flash Please, note in the video that front ESCs run cooler than back ones, so it could be interesting to be able to check individual ESC temperature during flights. It would require to also update the configurator.

github-actions[bot] commented 1 year ago

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

github-actions[bot] commented 1 year ago

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

haslinghuis commented 1 year ago

Seems configurator is already reading temperature telemetry data for individual motors here:

            case MSPCodes.MSP_MOTOR_TELEMETRY:
                const telemMotorCount = data.readU8();
                for (let i = 0; i < telemMotorCount; i++) {
                    FC.MOTOR_TELEMETRY_DATA.rpm[i] = data.readU32();   // RPM
                    FC.MOTOR_TELEMETRY_DATA.invalidPercent[i] = data.readU16();   // 10000 = 100.00%
                    FC.MOTOR_TELEMETRY_DATA.temperature[i] = data.readU8();       // degrees celsius
                    FC.MOTOR_TELEMETRY_DATA.voltage[i] = data.readU16();          // 0.01V per unit
                    FC.MOTOR_TELEMETRY_DATA.current[i] = data.readU16();          // 0.01A per unit
                    FC.MOTOR_TELEMETRY_DATA.consumption[i] = data.readU16();      // mAh
                }
                break;
damosvil commented 1 year ago

@haslinghuis Ok, I will update it. Thanks for notifying

damosvil commented 1 year ago

@haslinghuis Please, let me know if is correct. I created a branch before squashing in case something go wrong.

haslinghuis commented 1 year ago

Oh no, Git is hard to grasp as this went wrong. Please see https://github.com/betaflight/betaflight/wiki/Betaflight-Contribution#squash-your-commits as these are my work notes.

As you already committed you would something like https://github.com/betaflight/betaflight/wiki/Betaflight-Contribution#unwanted-commits-in-your-latest-push

damosvil commented 1 year ago

@haslinghuis I hope this time goes fine. Please, let me know if not

KarateBrot commented 1 year ago

@damosvil It's working now 👍

Now you need to rebase on current BF master to resolve the issue with src/main/drivers/dshot_command.c. It's coming from this PR which got merged into master just a few days ago: https://github.com/betaflight/betaflight/pull/11672

KarateBrot commented 1 year ago

Some unit test is failing and needs some love.

Could you change your commit name to something more descriptive, for example "Extended Bidir DShot" or whatever when it's ready to merge?

github-actions[bot] commented 1 year ago

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

damosvil commented 1 year ago

Some unit test is failing and needs some love.

Could you change your commit name to something more descriptive, for example "Extended Bidir DShot" or whatever when it's ready to merge?

@KarateBrot Fixed

github-actions[bot] commented 1 year ago

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

damosvil commented 1 year ago

be3ba50

Tests:

FPVM/BETAFLIGHTF4(STM32F405) target & Bluejay on 4xLANRC 35A individual ESCs

IFLIGHT_H743_AIO(STM32H743) target & Bluejay on JHEMCU EM40A 4in1 ESC

IFLIGHT TITAN DC5 (STM32F722) target & Blheli_32 IFLIGHT_G071_4IN1

github-actions[bot] commented 1 year ago

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

github-actions[bot] commented 1 year ago

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!