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.

Sign up for GitHub

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

Jump to bottom

Feat/prelight additions #7

Open
wants to merge 1 commit into
base: CMCopter-4.4
Choose a base branch
from
Open

Feat/prelight additions #7

wants to merge 1 commit into from

Conversation

mwls-sean
Copy link

@mwls-sean mwls-sean commented Nov 20, 2023 •
edited
Loading

Added percent brightness support to the SHOW.PRE_LIGHTS param. Existing integer values retain their level of brightness.

@mwls-sean mwls-sean changed the base branch from master to CMCopter-4.4 November 20, 2023 14:50
@CLAassistant
Copy link

CLAassistant commented Nov 20, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

mwls-sean
mwls-sean commented Nov 20, 2023
View reviewed changes
libraries/AC_DroneShowManager/AC_DroneShowManager.cpp
@@ -353,6 +353,7 @@ const AP_Param::GroupInfo AC_DroneShowManager::var_info[] = {
static DroneShowLEDFactory _rgb_led_factory_singleton;

static bool is_safe_to_change_start_time_in_stage(DroneShowModeStage stage);
static bool are_almost_equal(float a, float b);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler didn't like comparisons of variables to float values. Maybe there is a utility that already provides this?

@ntamas
Copy link
Member

ntamas commented Dec 26, 2023

Judging from the code in AP_Param.cpp, the raw storage format of a parameter in the EEPROM depends on its type. This PR changes the type of the SHOW_PRE_LIGHTS parameter from AP_Int8 to AP_Float so I think that previous values of this parameter saved into EEPROM would not be preserved if we merge this PR and a user upgrades the firmware on a flight controller from an earlier version of the Skybrush firmware to this one.

I'm putting this PR on hold for the time being. If there's considerable interest from the community to merge this feature, drop a message here. In that case, some kind of an upgrade mechanism will have to be provided that re-writes the type of the parameter in the EEPROM from AP_Int8 to AP_Float.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mwls-sean @CLAassistant @ntamas @seankovacs