REBOOT_REQUIRED flag to cms power options, unified confirm page options #9518

asizon · 2020-02-26T09:14:27Z

Added REBOOT_REQUIRED flag to all Power options in cms for changes to take efect. Also unified all CONFIRM pages options with YES and NO.

mikeller · 2020-02-26T12:43:22Z

src/main/cms/cms_menu_power.c

-    { "V METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_voltageMeterSource, VOLTAGE_METER_COUNT - 1, voltageMeterSourceNames }, 0 },
-    { "I METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_currentMeterSource, CURRENT_METER_COUNT - 1, currentMeterSourceNames }, 0 },
+    { "V METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_voltageMeterSource, VOLTAGE_METER_COUNT - 1, voltageMeterSourceNames }, REBOOT_REQUIRED },
+    { "I METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_currentMeterSource, CURRENT_METER_COUNT - 1, currentMeterSourceNames }, REBOOT_REQUIRED },


Do all of these really need a reboot?

No sorry, I mainly thought that they affected to real time current/voltage reads, but that is not the case.

Can you please point out what reference to batteryConfig()->voltageMeterSource and batteryConfig()->vbatmaxcellvoltage means that a reboot is required for changes to them to take effect?

The reality is that we are trying to move away from any settings changes requiring a reboot - most of them have been refactored to not require a reboot, and we should try to not muddy the waters by adding required reboots for settings that don't need them.

@mikeller Hi i noticed this update in 4.1.5 betaflight Target: TMOTORF7
Manufacturer: TMTR
Version: 4.1.5
Binary: betaflight_4.1.5_STM32F7X2.hex
Date: 16-03-2020 16:50

I am not sure if this is related, but i posted an issue .. WHat happens is Battery readings freeze as soon as quad is armed, readings dont happen i can only see the original battery level. If i unplug battery and re-plug i will only then see the new updated batt levels, again if i re-arm the quad voltage meters freeze again. please help me as i am close to damaging lipos. I use the T-Motor HD F7 Stack and 55a 4in1 32bit esc + DJI system

asizon · 2020-02-27T16:13:50Z

@mikeller definitely VBAT CLMAX also need a reboot because it aplies on first boot, I need to change it.

mikeller · 2020-02-28T22:58:09Z

src/main/cms/cms_menu_power.c

-    { "V METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_voltageMeterSource, VOLTAGE_METER_COUNT - 1, voltageMeterSourceNames }, 0 },
-    { "I METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_currentMeterSource, CURRENT_METER_COUNT - 1, currentMeterSourceNames }, 0 },
+    { "V METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_voltageMeterSource, VOLTAGE_METER_COUNT - 1, voltageMeterSourceNames }, REBOOT_REQUIRED },
+    { "I METER", OME_TAB, NULL, &(OSD_TAB_t){ &batteryConfig_currentMeterSource, CURRENT_METER_COUNT - 1, currentMeterSourceNames }, REBOOT_REQUIRED },


Can you please point out what reference to batteryConfig()->voltageMeterSource and batteryConfig()->vbatmaxcellvoltage means that a reboot is required for changes to them to take effect?

The reality is that we are trying to move away from any settings changes requiring a reboot - most of them have been refactored to not require a reboot, and we should try to not muddy the waters by adding required reboots for settings that don't need them.

asizon · 2020-02-29T06:57:07Z

@mikeller So maybe we should check vmaxcellvoltage outside of CMS? The truth is that I am not able to understund the code of these parameters, I rely on direct tests in cms, when I modify vmax for example by lowering it to 300 nothing changes until I apply Reboot, it is when it detects the vmax voltage and it gives me low warning battery, same for voltageMeterSource.

mikeller · 2020-02-29T21:50:03Z

@asizon:

I rely on direct tests in cms, when I modify vmax for example by lowering it to 300 nothing changes until I apply Reboot

Not sure what you mean by this - maybe you can try describing this as a bug:

Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Steps to reproduce the behavior:

Expected behavior
A clear and concise description of what you expected to happen.

Additional context
Add any other context about the problem here.

asizon · 2020-03-01T07:46:52Z

I hope this can clarify it @mikeller
Describe the bug
Reboot needed when I try to change VBAT CLMAX param

To Reproduce
Put 4s full battery , reduce default 430 CLMAX value to 330 value and check save and exit. The battery status is not affected, it still shows Battery Full.

Expected behavior
When you change the value of clmax to 330 which is a value lower than the maximum of my battery it should show Low battery and flickers in the voltage, it only does so if reboot is applied.

mikeller · 2020-03-01T07:57:48Z

Put 4s full battery , reduce default 430 CLMAX value to 330 value and check save and exit. The battery status is not affected, it still shows Battery Full.

I do not quite understand where it shows 'battery full'. Can you please be a bit more precise and specify what 'it' is, and maybe supply a screenshot / image of 'it'?

When you change the value of clmax to 330 which is a value lower than the maximum of my battery it should show Low battery and flickers in the voltage, it only does so if reboot is applied.

Not sure what you mean to achieve by setting the maximum cell voltage to 3.3V, to my knowledge there is no battery used in RC racing that has a cell voltage when full of 3.3V. What you are looking at after the reboot is a warning message because you have horribly misconfigured your craft, and the cell count detection was broken by it. There is no need for a 'fix', instead you should stop using utterly incorrect settings, and instead correctly configure your hardware.

asizon · 2020-03-01T08:07:32Z

I really don't know how much this is used, I just run this test to verify that the changes only came into effect when the flight controller was rebooted, here a sample image:

CLMAX 330 without reboot:

CLMAX 330 REBOOT aplies:

mikeller · 2020-03-01T08:17:47Z

Can you again specify what variables you think were only changed after the reboot and point them out in the code?

For what I understand is that you are using CMS to set absurdly wrong values, like a 'maximum cell voltage' that is ~1 V lower than the actual cell voltage. For this situation there is no 'expected behaviour', as this is so far outside of what the firmware was designed to support that it will behave completely errorneous.

asizon · 2020-03-01T08:21:46Z

OK @mikeller , now I begin to understand you, this applies before reboot, in the reboot other factors are applied that are incompatible with those absurd values. Sorry for this mixunderstunding.

Only for voltage/current meter source Add flag to vbat clmax Clmax flag revert

asizon · 2020-03-01T08:26:44Z

Removed CLMAX flag and rebased!

mikeller · 2020-03-01T08:44:47Z

@asizon: batteryConfig()->voltageMeterSource is the same - I could not find a single reference for its use that is exclusive to the initialisation, so I don't think it needs to require a reboot.

asizon · 2020-03-01T08:57:05Z

@mikeller for batteryConfig()->voltageMeterSource reboot is needed only when we want to use ESC SENSOR as voltage meter source.

mikeller · 2020-03-01T09:08:38Z

@asizon:

for batteryConfig()->voltageMeterSource reboot is needed only when we want to use ESC SENSOR as voltage meter source.

Are you sure? I do not have a test setup for this now, but to my recollection switching to / from ESC_SENSOR as the voltage meter source works without a reboot if the ESC_SENSOR feature is enabled and a port for it is configured.

asizon · 2020-03-01T09:11:49Z

Yes im sure, I tried it now, port configured, bidir disabled and esc_sensor enabled, setup voltage meter source to None, when I set to esc_sensor and save and exit it continues in 0.00v,only take efect after reboot and shows real voltage. For ADC meter source it works as expected.

asizon · 2020-03-01T09:16:38Z

Also verified it now in configurator to make sure that ESC_SENSOR is already well configured by cms and working properly.

mikeller · 2020-03-01T09:17:10Z

Heh, good point. If it is set to 'NONE' to start with it will not start the voltage monitor task.

REBOOT_REQUIRED flag to cms power options, unified confirm page options

mikeller reviewed Feb 26, 2020

View reviewed changes

mikeller previously approved these changes Feb 27, 2020

View reviewed changes

mikeller added this to the 4.1.5 milestone Feb 27, 2020

mikeller added the RN: BUGFIX label Feb 27, 2020

asizon dismissed mikeller’s stale review via dd1752e February 27, 2020 20:40

mikeller requested changes Feb 28, 2020

View reviewed changes

Reboot required flag to cms power options

66dbbe8

Only for voltage/current meter source Add flag to vbat clmax Clmax flag revert

asizon force-pushed the cmsImprovements branch from dd1752e to 66dbbe8 Compare March 1, 2020 08:25

mikeller approved these changes Mar 1, 2020

View reviewed changes

mikeller merged commit 86b81ba into betaflight:master Mar 1, 2020

mikeller added a commit that referenced this pull request Mar 1, 2020

Merge pull request #9518 from Asizon/cmsImprovements

46f22bc

REBOOT_REQUIRED flag to cms power options, unified confirm page options

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REBOOT_REQUIRED flag to cms power options, unified confirm page options #9518

REBOOT_REQUIRED flag to cms power options, unified confirm page options #9518

asizon commented Feb 26, 2020

mikeller Feb 26, 2020

asizon Feb 26, 2020

mikeller Feb 28, 2020

btemtd Mar 23, 2020

asizon commented Feb 27, 2020

mikeller Feb 28, 2020

asizon commented Feb 29, 2020

mikeller commented Feb 29, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020 •

edited

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

REBOOT_REQUIRED flag to cms power options, unified confirm page options #9518

REBOOT_REQUIRED flag to cms power options, unified confirm page options #9518

Conversation

asizon commented Feb 26, 2020

mikeller Feb 26, 2020

Choose a reason for hiding this comment

asizon Feb 26, 2020

Choose a reason for hiding this comment

mikeller Feb 28, 2020

Choose a reason for hiding this comment

btemtd Mar 23, 2020

Choose a reason for hiding this comment

asizon commented Feb 27, 2020

mikeller Feb 28, 2020

Choose a reason for hiding this comment

asizon commented Feb 29, 2020

mikeller commented Feb 29, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020 • edited

asizon commented Mar 1, 2020

mikeller commented Mar 1, 2020

asizon commented Mar 1, 2020 •

edited