betaflight icon indicating copy to clipboard operation
betaflight copied to clipboard

Extend build info with defined flags

Open atomgomba opened this issue 1 year ago • 32 comments

This PR extends the MSP_BUILD_INFO command response with a few bytes as a way to make the firmware able to communicate the build options it was built with to clients, thus eliminating the need to have a working internet connection to retrieve this info. One of the main motivations for this is to make the Configurator work better when offline and make the TX lua scripts more efficient by knowing what information it can retrieve and display. Hopefully this will make the TX setup tool work much faster. The current list of options was retrieved from this endpoint.

atomgomba avatar Jan 28 '24 19:01 atomgomba

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #13333 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

github-actions[bot] avatar Jan 28 '24 19:01 github-actions[bot]

Another possible implementation:

  • augment https://build.betaflight.com/api/options/4.5.0-zulu with group and option id's (small integer that is not reused on protocol changes)
  • add python (or perl) code generator to BF, together with generated files (and preferable add makefile rules)
  • use slightly more general structure for encoding, for example ( <group-id:5bits; group-lenght-bytes:3bits><group bitdata><next-group-id+length ... ><0x00>)
  • state that <group bitdata> may be defined specially for given group-id. Configurator will skip unknown group-ids / more efficient encoding for group-id may be defined in future

ledvinap avatar Jan 29 '24 17:01 ledvinap

Sorry guys this has to be a 4.6 feature I think.

ctzsnooze avatar Jan 30 '24 00:01 ctzsnooze

Another possible implementation:

  • augment https://build.betaflight.com/api/options/4.5.0-zulu with group and option id's (small integer that is not reused on protocol changes)
  • add python (or perl) code generator to BF, together with generated files (and preferable add makefile rules)
  • use slightly more general structure for encoding, for example ( <group-id:5bits; group-lenght-bytes:3bits><group bitdata><next-group-id+length ... ><0x00>)
  • state that <group bitdata> may be defined specially for given group-id. Configurator will skip unknown group-ids / more efficient encoding for group-id may be defined in future

I wonder if - given the sprawl of options will likely far exceed the options actually selected by the user - we simply assign a 2 byte number to each option (could be more efficient, but 1 byte is insufficient) and then simply stream these in the MSP response. We could as you have suggested assign a grouping for the first few bits (RX, GPS, MOTOR etc).

Only those selected will end up compiled, streamed and taking up flash / MSP message space.

Would also make the CLI output of the same human readable (at least numerically).

blckmn avatar Jan 30 '24 20:01 blckmn

Thank you for your valuable input and inspiring ideas! I know currently this is a very simplistic approach, but my most important concern was the amount of bytes transferred with over-the-air usage in mind to make configuration on the field more responsive by reducing the time to initialize and reduce confusion by hiding unavailable options. Currently we can only do this by firing an MSP request per feature and wait for an error or success response and it is far from efficient so retrieving the list of possible options in one go will help a lot.

@ledvinap You are right, making it possible to generate code for this purpose was part of the reasons why I moved this into its separate file, so it is naturally doable. However it will introduce its own problems in the build tooling of course.

I really like the idea of skippable groups (e.g. it's likely that "telemetry" options will be often skipped due to the popularity of CRSF), spending a header byte to indicate included groups seem worthy. Currently we have 4 groups, is it likely we'll ever extend beyond 8 groups? Also spending 2 bytes per gate feels like an overkill. Say the user will likely select at least 5-6 gates (currently there are 9 gates set as default if I'm not mistaken) which is already 10-18 bytes while the code in this PR would just send a fixed 6 bytes for all 43 gates (7, 8 .. bytes once we extend beyond 48, 56 .. gates).

Although it is also true that @blckmn 's idea would make the protocol really simple in contrast to the more general group structure proposal. At the moment I'm failing to see a good use case for letting the internal group data a custom structure depending on the group, considering that gates are really just true/false values. To use the response implemented in this PR could be processed on the lua side as simply as the following:

// test availability of OSD SD
local dateLength = 11
local timeLength = 8
local revisionLength = 7
// payload is the MSP response
local optionByte4 = payload[dateLength + timeLength + revisionLength + 4]
local hasOSDSD = bit32.band(optionByte4, 0x07) ~= 0

But of course the drawback of using arbitrarily ordered values remains. I'm totally open to improve on this simplistic approach, but somewhat confused about how to balance between a more sane structure and still keeping a minimal payload size, and perhaps not complicating encoding/decoding too much either...

atomgomba avatar Jan 31 '24 07:01 atomgomba

I would not worry too much about payload size. It's a once per connection request response.

I'm more concerned with flash space and maintainability.

blckmn avatar Jan 31 '24 07:01 blckmn

I would not worry too much about payload size. It's a once per connection request response.

I'm more concerned with flash space and maintainability.

Fine, that sounds convincing enough :) In this case I'm planning to go with the 2 byte indices approach, if that's okay

atomgomba avatar Jan 31 '24 08:01 atomgomba

@atomgomba:

Something line this may work:

static const uint16_t build_options[] = {
#ifdef USE_SERIALRX_FPORT
    BUILD_OPTION_SERIALRX_FPORT, 
#endif
#ifdef ... 
};

And then use ARRAYLEN() to send build_options ...

ledvinap avatar Jan 31 '24 10:01 ledvinap

@atomgomba:

Something line this may work:

static const uint16_t build_options[] = {
#ifdef USE_SERIALRX_FPORT
    BUILD_OPTION_SERIALRX_FPORT, 
#endif
#ifdef ... 
};

And then use ARRAYLEN() to send build_options ...

Exactly this. The header file setting the BUILD_OPTION_SERIALRX_FPORT value can be generated (at least initially), and I will update the API to also return the "values" when https://build.betaflight.com/api/options/4.5.0 or similar is obtained.

blckmn avatar Jan 31 '24 20:01 blckmn

Yes, I think we're on the same page. Currently I have this sketch (please see last commit), not tested yet. I used this script for generating the code. I realized if I used defines instead of an enum then unused values won't be included in the compiled binary, if I'm correct. EDIT: Or would the compiler optimise for size and it won't matter anyway?

atomgomba avatar Jan 31 '24 20:01 atomgomba

Defines are fine. Please note I have added the keys (which includes the grouping - using 4 high order bits, allowing for up to 15 groups, and 4095 total options) into the API, and so you can simply use those values.

see here: https://build.betaflight.com/api/options/4.5.0

blckmn avatar Jan 31 '24 21:01 blckmn

// Radio Protocols
#DEFINE  BUILD_OPTION_SERIALRX_CRSF               4097
#DEFINE  BUILD_OPTION_SERIALRX_FPORT              4098
#DEFINE  BUILD_OPTION_SERIALRX_GHST               4099
#DEFINE  BUILD_OPTION_SERIALRX_IBUS               4100
#DEFINE  BUILD_OPTION_SERIALRX_JETIEXBUS          4101
#DEFINE  BUILD_OPTION_RX_PPM                      4102
#DEFINE  BUILD_OPTION_SERIALRX_SBUS               4103
#DEFINE  BUILD_OPTION_SERIALRX_SPEKTRUM           4104
#DEFINE  BUILD_OPTION_SERIALRX_SRXL2              4105
#DEFINE  BUILD_OPTION_SERIALRX_SUMD               4106
#DEFINE  BUILD_OPTION_SERIALRX_SUMH               4107
#DEFINE  BUILD_OPTION_SERIALRX_XBUS               4108

// Telemetry Protocols
#DEFINE  BUILD_OPTION_TELEMETRY_FRSKY_HUB        12301
#DEFINE  BUILD_OPTION_TELEMETRY_HOTT             12302
#DEFINE  BUILD_OPTION_TELEMETRY_IBUS_EXTENDED    12303
#DEFINE  BUILD_OPTION_TELEMETRY_LTM              12304
#DEFINE  BUILD_OPTION_TELEMETRY_MAVLINK          12305
#DEFINE  BUILD_OPTION_TELEMETRY_SMARTPORT        12306
#DEFINE  BUILD_OPTION_TELEMETRY_SRXL             12307

// General Options
#DEFINE  BUILD_OPTION_ACRO_TRAINER               16404
#DEFINE  BUILD_OPTION_AKK_SMARTAUDIO             16405
#DEFINE  BUILD_OPTION_BATTERY_CONTINUE           16406
#DEFINE  BUILD_OPTION_CAMERA_CONTROL             16407
#DEFINE  BUILD_OPTION_DASHBOARD                  16408
#DEFINE  BUILD_OPTION_EMFAT_TOOLS                16409
#DEFINE  BUILD_OPTION_ESCSERIAL_SIMONK           16410
#DEFINE  BUILD_OPTION_FRSKYOSD                   16411
#DEFINE  BUILD_OPTION_GPS                        16412
#DEFINE  BUILD_OPTION_LED_STRIP                  16413
#DEFINE  BUILD_OPTION_LED_STRIP_64               16414
#DEFINE  BUILD_OPTION_MAG                        16415
#DEFINE  BUILD_OPTION_OSD_SD                     16416
#DEFINE  BUILD_OPTION_OSD_HD                     16417
#DEFINE  BUILD_OPTION_PINIO                      16418
#DEFINE  BUILD_OPTION_RACE_PRO                   16419
#DEFINE  BUILD_OPTION_SERVOS                     16420
#DEFINE  BUILD_OPTION_VTX                        16421

// Motor Protocols
#DEFINE  BUILD_OPTION_BRUSHED                     8230
#DEFINE  BUILD_OPTION_DSHOT                       8231
#DEFINE  BUILD_OPTION_MULTISHOT                   8232
#DEFINE  BUILD_OPTION_ONESHOT                     8233
#DEFINE  BUILD_OPTION_PROSHOT                     8234
#DEFINE  BUILD_OPTION_PWM_OUTPUT                  8235

blckmn avatar Jan 31 '24 22:01 blckmn

What do you think about the latest changes? I would like to make the changes in the lua scripts to take advantage of this, even if this won't be merged to master right away, I just would like to avoid back-and-forth modifications.

atomgomba avatar Feb 01 '24 17:02 atomgomba

While testing some fields are missing (for current logic):

image

Can we add:

USE_TELEMETRY_CFRF
USE_TELEMETRY_GHST
USE_TELEMETRY_FPORT
USE_TRANSPONDER

Maybe we also add

USE_RX_MSP
USE_RX_PARALLEL_PWM
USE_SPI_RX
USE_RX_SERIAL

Configurator depends on these to be available in features.js:

const features = [
        {bit: 0, group: 'rxMode', mode: 'select', name: 'RX_PPM'},
        {bit: 2, group: 'other', name: 'INFLIGHT_ACC_CAL'},
        {bit: 3, group: 'rxMode', mode: 'select', name: 'RX_SERIAL'},
        {bit: 4, group: 'escMotorStop', name: 'MOTOR_STOP'},
        {bit: 5, group: 'other', name: 'SERVO_TILT', haveTip: true, dependsOn: 'SERVOS'},
        {bit: 6, group: 'other', name: 'SOFTSERIAL', haveTip: true},
        {bit: 7, group: 'other', name: 'GPS', haveTip: true, dependsOn: 'GPS'},
        {bit: 9, group: 'other', name: 'SONAR', haveTip: true, dependsOn: 'RANGEFINDER'},
        {bit: 10, group: 'telemetry', name: 'TELEMETRY', haveTip: true, dependsOn: 'TELEMETRY'},
        {bit: 12, group: '3D', name: '3D', haveTip: true},
        {bit: 13, group: 'rxMode', mode: 'select', name: 'RX_PARALLEL_PWM'},
        {bit: 14, group: 'rxMode', mode: 'select', name: 'RX_MSP'},
        {bit: 15, group: 'rssi', name: 'RSSI_ADC'},
        {bit: 16, group: 'other', name: 'LED_STRIP', haveTip: true, dependsOn: 'LED_STRIP'},
        {bit: 17, group: 'other', name: 'DISPLAY', haveTip: true, dependsOn: 'DASHBOARD'},
        {bit: 18, group: 'other', name: 'OSD', haveTip: true, dependsOn: 'OSD'},
        {bit: 20, group: 'other', name: 'CHANNEL_FORWARDING', dependsOn: 'SERVOS'},
        {bit: 21, group: 'other', name: 'TRANSPONDER', haveTip: true, dependsOn: 'TRANSPONDER'},
        {bit: 22, group: 'other', name: 'AIRMODE'},
        {bit: 25, group: 'rxMode', mode: 'select', name: 'RX_SPI'},
        {bit: 27, group: 'escSensor', name: 'ESC_SENSOR'},
        {bit: 28, group: 'antiGravity', name: 'ANTI_GRAVITY', haveTip: true, hideName: true},
    ];

haslinghuis avatar Feb 02 '24 15:02 haslinghuis

I guess these are the ones that aren't part of auto-complete as they have their own UI widget for selecting them. Once they become included in the API, I can regenerate the list of defines :+1:

atomgomba avatar Feb 02 '24 16:02 atomgomba

@atomgomba : Can you please create PR for generator scripts?

ledvinap avatar Feb 02 '24 16:02 ledvinap

Master:

image

This PR and configurator PR:

image

haslinghuis avatar Feb 02 '24 17:02 haslinghuis

@atomgomba : Can you please create PR for generator scripts?

Sure thing, but it will need some more work to be truly useful. At the moment it just writes enumerations to files in the same directory and I copy-pasted the contents into the C source files. I'm guessing the best would be for it to reside under src/utils and directly replace msp_build_info.* files upon execution.

atomgomba avatar Feb 02 '24 17:02 atomgomba

Can we have support for https://github.com/betaflight/betaflight-configurator/pull/3583, meaning we need some groups as @haslinghuis comment, to support higerlevel groups the in image ie.

// Radio Protocols
#DEFINE  BUILD_GROUP_RADIO                        4000

// Telemetry Protocols
#DEFINE  BUILD_GROUP_TELEMETRY                   12000

// General Options
#DEFINE  BUILD_GROUP_GENERAL                     16000

// Motor Protocols
#DEFINE  BUILD_GROUP_MOTOR                        8200

HThuren avatar Feb 03 '24 15:02 HThuren

@HThuren Could you please elaborate? I don't understand why including the options that serve as input parameters to make isn't enough

atomgomba avatar Feb 03 '24 16:02 atomgomba

The group value is already contained in the value:

image

haslinghuis avatar Feb 03 '24 17:02 haslinghuis

@atomgomba, right, @haslinghuis show me groups, I can see they already are there. My point of view are to support when you setup in configurator, and here to help that user can't features not supported in build.

I will hope to have more groups, ie General splitted more, to cover, "Your solution" / "Suggestion" : image

HThuren avatar Feb 03 '24 17:02 HThuren

@HThuren Please note that Configurator-over-the-wire isn't the only potential consumer of this MSP reply. The response is already too verbose to my linking (originally I imagined transferring only a handful of bytes containing bit flags), I don't think we should send anything that can be included/calculated on the client side. OTA data transfer can be very slow already on some protocols and the whole point is to avoid sending one MSP command per feature to test their availability. If a single response becomes nearly as long as multiple other ones combined then it will defeat the whole purpose.

atomgomba avatar Feb 03 '24 18:02 atomgomba

@atomgomba maybe for LUA [we] need to introduce another MSP request for retrieving a specific build option instead of retrieving the whole list.

haslinghuis avatar Feb 03 '24 18:02 haslinghuis

@atomgomba maybe for LUA [we] need to introduce another MSP request for retrieving a specific build option instead of retrieving the whole list.

I'm sold. This is the way!

atomgomba avatar Feb 03 '24 18:02 atomgomba

This is the way!

HThuren avatar Feb 03 '24 18:02 HThuren

This last pushed script accepts an option for the URL to the endpoint (or defaults to the current one when omitted), and generates identical source files to the ones in this PR in place.

atomgomba avatar Feb 08 '24 17:02 atomgomba

One more thing: Generated files should contain info about data source. Using only endpoint may be enough, but better info will be better.

ledvinap avatar Feb 09 '24 11:02 ledvinap

@ledvinap Thank you for the review and valuable comments!

atomgomba avatar Feb 09 '24 15:02 atomgomba

I replaced the timestamp with a stable hash of the input data

atomgomba avatar Feb 09 '24 15:02 atomgomba