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 LED strip profile feature #7303

Merged
merged 1 commit into from Jan 19, 2019

Conversation

Projects
None yet
3 participants
@pkruger
Copy link
Contributor

pkruger commented Dec 28, 2018

Fixes: #4278

@pkruger pkruger force-pushed the pkruger:4278-LED-profiles-switchable-via-OSD branch 3 times, most recently from 24504f8 to 446885b Dec 28, 2018

@pkruger pkruger force-pushed the pkruger:4278-LED-profiles-switchable-via-OSD branch from 446885b to 7192fd9 Dec 29, 2018

@mikeller
Copy link
Member

mikeller left a comment

This has a lot of overlap with something that I've been working on: Creating a 'minimal LED_STRIP' compile time option. The reason for this is that we are constantly running out of flash space on F3, and had to disable LED_STRIP support on F3 entirely in master for this reason. We want to bring back a minimal version of LED strip support that caters to the needs of racers, but keep the bloat that the rest of the LED strip code brings out of it. Doing this by splitting it into different 'LED_STRIP profiles', and not supporting the STATUS profile on F3 sounds like an attractive option for this.
So what do you think of the following modification of your pull request: Change the conditionals so that USE_LED_STRIP enables only RACE and BEACON, and USE_LED_STRIP_STATUS_MODE enables the rest of the code (We'd probably want to change the of the options in the adjustment range as well to make STATUS come last.)
Additionally, I think it would be great if users can change the race colour without having to use CLI, or configurator. We could add this option to the CMS/OSD.

Show resolved Hide resolved src/main/io/ledstrip.c Outdated
Show resolved Hide resolved src/main/io/ledstrip.c Outdated

@mikeller mikeller added this to the 4.0 milestone Dec 29, 2018

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Dec 29, 2018

@mikeller Interesting idea above, good to get a view of the bigger picture. I'm happy to change it to what you suggested or if you are far along with your changes just to rather adopt that - not fussed at all. I'll have a look at changing to your suggestion in the mean while and add the review changes above. Also, the race profile colors can already be changed in the OSD and CLI with the above changes. (Will be on holidays for a few days so might be a little slow to respond.)

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 29, 2018

@pkruger: My work on this was only very preliminary, as I was not able to come up with a good way to do the user interface for this - your idea with the profiles (and having the RACE / BEACON mode not be part of the complex STATUS configuration) provides an elegant solution for the part where I got stuck. So I'd go for your changes, making them built always for LED_STRIP and making the existing complex solution optional should do the trick nicely.

P.S.: Enjoy your holiday.

@pkruger pkruger force-pushed the pkruger:4278-LED-profiles-switchable-via-OSD branch 3 times, most recently from 2cd67d5 to a6a24db Dec 29, 2018


#ifdef USE_LED_STRIP

static bool featureRead = false;
static uint8_t cmsx_FeatureLedstrip;
#ifdef USE_LED_PROFILES

This comment has been minimized.

@mikeller

mikeller Jan 9, 2019

Member

Not quite sure if we need USE_LED_PROFILES at all. If USE_LED_STRIP was defined, but not USE_LED_PROFILES, what could the user do with the LED_STRIP, and how would they configure it?

This comment has been minimized.

@pkruger

pkruger Jan 17, 2019

Author Contributor

@mikeller The idea with USE_LED_PROFILES is to cater for targets that will run out of space if this feature is added, i.e. they can still use the LED_STRIP functionality as is now. I agree we may not need it anyway since LED_STRIP will probably be removed then for other more important features. I think the cases covered by USE_LED_PROFILES can be handled by USE_LED_STRIP where necessary.

This comment has been minimized.

@mikeller

mikeller Jan 17, 2019

Member

@pkruger: This is the wrong way round. As it is, LED_STRIP was disabled for all F3 based targets early in 4.0 to (temporarily) make room for new development. As it stands, we do not have the space on F3 to re-introduce the bloated legacy LED_STRIP implementation, so what we will do is only add the two 'lightweight' profiles for F3 with USE_LED_STRIP, and only define USE_LED_STRIP_STATUS_MODE and enable the legacy configuration for F4.
We could leave USE_LED_PROFILES in as a conditional so that users can build a 'legacy mode only' version, but we are not planning on using it for any official targets

This comment has been minimized.

@pkruger

pkruger Jan 17, 2019

Author Contributor

@mikeller Ok, I understand. If we aren't planning to use the 'legacy mode only' version it's probably better to remove it. I'll have a look at removing it so only USE_LED_STRIP & USE_LED_STRIP_STATUS_MODE is used.

This comment has been minimized.

@pkruger

pkruger Jan 17, 2019

Author Contributor

Done.

@pkruger pkruger force-pushed the pkruger:4278-LED-profiles-switchable-via-OSD branch 5 times, most recently from 889c9c7 to 191cbe6 Jan 17, 2019

@@ -1013,8 +1046,12 @@ const clivalue_t valueTable[] = {
// PG_LED_STRIP_CONFIG
#ifdef USE_LED_STRIP
{ "ledstrip_visual_beeper", VAR_UINT8 | MASTER_VALUE | MODE_LOOKUP, .config.lookup = { TABLE_OFF_ON }, PG_LED_STRIP_CONFIG, offsetof(ledStripConfig_t, ledstrip_visual_beeper) },
#ifdef USE_LED_STRIP_STATUS_MODE

This comment has been minimized.

@mikeller

mikeller Jan 17, 2019

Member

ledstrip_grb_rgb is applied in the LED_STRIP driver, so it should be available even without USE_LED_STRIP_STATUS_MODE.

This comment has been minimized.

@pkruger

pkruger Jan 19, 2019

Author Contributor

Done.

@@ -76,42 +77,28 @@

#include "telemetry/telemetry.h"

PG_REGISTER_WITH_RESET_FN(ledStripConfig_t, ledStripConfig, PG_LED_STRIP_CONFIG, 0);
PG_REGISTER_WITH_RESET_FN(ledStripConfig_t, ledStripConfig, PG_LED_STRIP_CONFIG, 1);

This comment has been minimized.

@mikeller

mikeller Jan 17, 2019

Member

This shouldn't be needed - appending parameters to the end of a parameter group is supported without an increment of the version. See https://github.com/betaflight/betaflight/blob/master/docs/development/ParameterGroups.md

This comment has been minimized.

@pkruger

pkruger Jan 19, 2019

Author Contributor

Thanks - done.

@@ -80,3 +80,4 @@
#define TARGET_IO_PORTB 0xffff
#define TARGET_IO_PORTC 0xffff

#define USE_LED_STRIP_STATUS_MODE

This comment has been minimized.

@mikeller

mikeller Jan 17, 2019

Member

Please move this near USE_LED_STRIP.

This comment has been minimized.

@pkruger

pkruger Jan 19, 2019

Author Contributor

Done.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Jan 17, 2019

@pkruger: Some tests are failing.

@pkruger pkruger force-pushed the pkruger:4278-LED-profiles-switchable-via-OSD branch from 191cbe6 to eabc5c3 Jan 17, 2019

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Jan 17, 2019

@mikeller Hmm.. I'll have a look. I get different errors if I try to run make test locally.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Jan 17, 2019

@pkruger: The tests build with the locally installed gcc, and not the version-checked arm gcc, so that might be the cause for the different error messages.

@pkruger pkruger force-pushed the pkruger:4278-LED-profiles-switchable-via-OSD branch from eabc5c3 to 75ecdc8 Jan 18, 2019

@pkruger pkruger force-pushed the pkruger:4278-LED-profiles-switchable-via-OSD branch 2 times, most recently from 8bb14f5 to 4ec536a Jan 18, 2019

@mikeller mikeller merged commit dfb438c into betaflight:master Jan 19, 2019

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Jan 19, 2019

Whoo hoo done...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.