Skip to content
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

Add strobe overlay to RGB LEDs #8536

Merged
merged 40 commits into from Jan 20, 2023
Merged

Conversation

mmosca
Copy link
Collaborator

@mmosca mmosca commented Nov 8, 2022

This is a change to enable than adding a Strobe blink as a new overlay, similar to #8531

ledConfig_t had no bits free to add new overlays, requiring it to be changed packed struct. which ends up being 40bits (as compared to the current 32).

This change introduces a new 'E' overlay for strob'E' which can be enabled as follows from the cli.

You have 2 leds in your chain, you can use the following command to set them as navigation + strobe lights:

# Led 0, Color + Strobe, Color 5 (green)
led 0 0,0::CE:5
# Led 1, Color + Strobe, Color 2 (red)
led 1 0,1::CE:2

New MSP commands have been added to GET/SET leds and the old MSP command is still supported, but won't allow you to set the strobe mode.

A corresponding pull request for the configurator has also been added that uses the new MSP commands and allows setting the strobe overlay from the usual led UI.

@MrD-RC MrD-RC added Requires Configurator Release Notes Add this when a PR needs to be mentioned in the release notes Enhancement labels Nov 8, 2022
@mmosca mmosca mentioned this pull request Nov 8, 2022
@mmosca
Copy link
Collaborator Author

mmosca commented Nov 8, 2022

@MrD-RC
Copy link
Collaborator

MrD-RC commented Nov 9, 2022

I think that this is the better way of doing it. Changing a LED mode that people already use to blink differently is not the way to go. It should be a new mode, as in this PR. This will also need changes to MSP elsewhere. The Configurator definitely.

@stronnag Anything else that my need attention?

@stronnag
Copy link
Collaborator

stronnag commented Nov 9, 2022

We try to maintain the MSP public API (as we have no idea who might be using it). Albeit, in this case, I doubt we have many other consumers beyond the configurator.

Historically, We've used one of two approaches:

  • Differentiate by length. This doesn't work here as there is both a getter and setter
  • Introduce a new (MSPv2) EX(tended) definition and support both old & new (e.g.: new API:
    • MSP2_LED_STRIP_CONFIG_EX
    • MSP2_SET_LED_STRIP_CONFIG_EX

We can always deprecate the old API and remove it in a future release.

Changing a public API without prior deprecation notice is a bit rude.

As the configurator only supports the latest firmware release, it only needs to support the new API.

@mmosca
Copy link
Collaborator Author

mmosca commented Nov 9, 2022

@stronnag Is there a rule on how to pick the id for the new _EX commands? Would 248 and 249 be ok? They appear unused.

I will keep the old ones working and they will just convert to/from the new struct and drop the strobe property in the process.

Also, Should I bump the minor API version, since we are adding 2 new functions?

@mmosca
Copy link
Collaborator Author

mmosca commented Nov 9, 2022

I can confirm the the legacy commands work. The 6.0.0-FP2 configurator can successfully read/write LED configuration.

Distributing new bits now gives us some room to implement some new functions and overlays without needing to change the data format.

Add 4 bits to functions
Add 2 bits to Overlay (one is used by strobe, aonther one is free)
Add 2 bits to params
@mmosca
Copy link
Collaborator Author

mmosca commented Nov 9, 2022

Configurator changes have been submitted in iNavFlight/inav-configurator#1647 and sucessfully tested locally.

@MrD-RC MrD-RC added this to the 6.0 milestone Nov 27, 2022
@DzikuVx DzikuVx merged commit 24bf2f8 into iNavFlight:master Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Release Notes Add this when a PR needs to be mentioned in the release notes Requires Configurator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants