OpenSourceEBike / TSDZ2_wireless

TSDZ2_wireless
35 stars 11 forks source link

walk mode working #85

Closed rananna closed 3 years ago

rananna commented 3 years ago

@casainho ,

Walk mode is now working well for the wireless remote

rananna commented 3 years ago

@casainho , @4var1 ,

I started a pwr analysis for this release candidate and was VERY surprised to see over 10X increase in power consumption. I traced it down to the pwm code, specifically the led init function (led_init). I attached two traces, one with this function enabled, and one without. with_led_init no_led_init

obviously, the repeating timer is a BIG power hog. This wipes out any savings from using pwm. Sorry to be the bearer of bad news, but we need to rethink what we are doing.

rananna commented 3 years ago

Can we turn the pwm timer on ONLY when the led is on?

casainho commented 3 years ago

What is the difference from current implementation and previous one, with the PWM low power from the Nordic?

rananna commented 3 years ago

What is the difference from current implementation and previous one, with the PWM low power from the Nordic?

Unfortunately, I do not know. With all the problems I was having with interrupts with the Nordic implementation, I never got around to doing a power analysis.

casainho commented 3 years ago

What I think is that regular PWM uses a lot of power. But "PWM low" from the Nordic example code uses a TIMER_APP to achieve low power. I would say we need to learn with Nordic "PWM low" example code...

rananna commented 3 years ago

Yes, the repeating timers used in the pwm code are big power hogs. There seem to be two used, one for pwm and the other copied from the wireless controller that counts time (main ticks) before incorporating the pwm code I did not have any repeating timers in the wireless remote. Getting dimmer LEDs by consuming 10x more power isn't a very good tradeoff :)

4var1 commented 3 years ago

I'm not entirely surprised so it's not that much bad news - I always thought there was a significant possibly the ISR would suck up power - so there are a few things we can do - i'll also have a look at the nordic code

  1. Reduce the pwm frequency. This can be done with the same level of flicker - but it would mean that the LED is on for a greater % of the time. At present the LED (for lowest brightness) is lit for 1/16 of the time. If we go for 1/8 i can halve the interrupt timer frequency etc. Also means fewer brightness levels are possible - but I think we only really need two at a push... dim and bright
  2. Move the led sequencing processing to a separate timer. There is code that runs every interrupt at the moment to compare a timer and only run the led sequence code every 50mS - this isn't very efficient. Most efficient would be for the led sequence to only be called when necessary and not be on a timer at all - this is easy in the controller as there is already a 50mS loop.
  3. Make the interrupt only enable when the LED is lit - that was one of my original suggestions - and would be the biggest reducer of power consumed by app timer interrupts.

On Wed, 17 Feb 2021 at 16:20, casainho notifications@github.com wrote:

What I think is that regular PWM uses a lot of power. But "PWM low" from the Nordic example code uses a TIMER_APP to achieve low power. I would say we need to learn with Nordic "PWM low" example code...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/85#issuecomment-780673155, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASRDIVT4PY75UQGZAHWN5PTS7PUEDANCNFSM4XYJDMEA .

4var1 commented 3 years ago

I'll have some time later - so will try out a few options - and hopefully have something to power test.

On Wed, 17 Feb 2021 at 17:28, Ben Machin ben.machin@gmail.com wrote:

I'm not entirely surprised so it's not that much bad news - I always thought there was a significant possibly the ISR would suck up power - so there are a few things we can do - i'll also have a look at the nordic code

  • see if there is anything unusual they are doing.
  1. Reduce the pwm frequency. This can be done with the same level of flicker - but it would mean that the LED is on for a greater % of the time. At present the LED (for lowest brightness) is lit for 1/16 of the time. If we go for 1/8 i can halve the interrupt timer frequency etc. Also means fewer brightness levels are possible - but I think we only really need two at a push... dim and bright
  2. Move the led sequencing processing to a separate timer. There is code that runs every interrupt at the moment to compare a timer and only run the led sequence code every 50mS - this isn't very efficient. Most efficient would be for the led sequence to only be called when necessary and not be on a timer at all - this is easy in the controller as there is already a 50mS loop.
  3. Make the interrupt only enable when the LED is lit - that was one of my original suggestions - and would be the biggest reducer of power consumed by app timer interrupts.

On Wed, 17 Feb 2021 at 16:20, casainho notifications@github.com wrote:

What I think is that regular PWM uses a lot of power. But "PWM low" from the Nordic example code uses a TIMER_APP to achieve low power. I would say we need to learn with Nordic "PWM low" example code...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/85#issuecomment-780673155, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASRDIVT4PY75UQGZAHWN5PTS7PUEDANCNFSM4XYJDMEA .

4var1 commented 3 years ago

@rananna can you see how the power consumption is split between running the processor and lighting the LED by any chance?

rananna commented 3 years ago

If our objective in dimming the LEDs is to reduce power consumption, maybe even using pwm ONLY when the LED is on may not even achieve any significant power savings over full on static DC current. Perhaps the timer would still consume a lot of processing power when the led is on, even more than the static DC needed to drive the LED. We might even be better off with full on DC only. @4var1, if you want to create a bit of code to compare DC to any pwm code revision you may create, I could run the power comparison.

(Of course, the other advantage of pwm is to even out the brightness variations between R, G, and B., and offer the possibility of dimmer LEDs at night or low brightness conditions. If we can get the per issue addressed, it would still be better to gave it than not....)

rananna commented 3 years ago

I'm not entirely surprised so it's not that much bad news - I always thought there was a significant possibly the ISR would suck up power - so there are a few things we can do - i'll also have a look at the nordic code - see if there is anything unusual they are doing. 1. Reduce the pwm frequency. This can be done with the same level of flicker - but it would mean that the LED is on for a greater % of the time. At present the LED (for lowest brightness) is lit for 1/16 of the time. If we go for 1/8 i can halve the interrupt timer frequency etc. Also means fewer brightness levels are possible - but I think we only really need two at a push... dim and bright 2. Move the led sequencing processing to a separate timer. There is code that runs every interrupt at the moment to compare a timer and only run the led sequence code every 50mS - this isn't very efficient. Most efficient would be for the led sequence to only be called when necessary and not be on a timer at all - this is easy in the controller as there is already a 50mS loop. 3. Make the interrupt only enable when the LED is lit - that was one of my original suggestions - and would be the biggest reducer of power consumed by app timer interrupts. On Wed, 17 Feb 2021 at 16:20, casainho @.***> wrote: What I think is that regular PWM uses a lot of power. But "PWM low" from the Nordic example code uses a TIMER_APP to achieve low power. I would say we need to learn with Nordic "PWM low" example code... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#85 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASRDIVT4PY75UQGZAHWN5PTS7PUEDANCNFSM4XYJDMEA .

Regarding 2. I would prefer we eliminate the need for a 50ms repeating timer in the remote as it is not needed for anything but pwm and it is also a power hog.

rananna commented 3 years ago

@rananna can you see how the power consumption is split between running the processor and lighting the LED by any chance?

Yes, it behaves like an oscilloscope, so if you modified the pwm to be active only when the led is lit, we can see the consumption before, during and after pwm. If you put in some test code to simply turn on the LEDs just after using pwm, we can also see the current difference there as well.

4var1 commented 3 years ago

I didn't know the nrf has two LEDs... or a reset button for that matter! Was looking to see the power spec of the RGB led - the APA102 leds I use have a max draw of 100mA so hoping it's less... :-o

https://infocenter.nordicsemi.com/index.jsp?topic=%2Fug_nrf52840_dongle%2FUG%2Fnrf52840_Dongle%2Fhw_button_led.html

4var1 commented 3 years ago

@rananna - as a quick test (but you'll still have the app timers running) - just change the bitstripe for each colour for the brightness you want to all 1's in ledalert.c - array defs near the top. Then it's still doing pwm - but never switches off the LED so effectively DC.

I'll be back in an hour or so and will start looking at improving the power utilisation!

4var1 commented 3 years ago

uint16_t ui16_pwm_table_red[LED_PWM_TABLE_LEN] = { 0b0000000000000000, // 0 pulses/mS 0b0000000100000001, // 2 pulses/mS 0b0100010000100010, // 4 pulses/mS 0b1001001001010100, // 6 pulses/mS 0b1010101010101010, // 8 pulses/mS 0b1110101011101010, // 10 pulses/mS 0b1011101011101111, // 12 pulses/mS 0b1111101111101111, // 14 pulses/mS };

uint16_t ui16_pwm_table_green[LED_PWM_TABLE_LEN] = //Make green less bright { 0b0000000000000000, // 0 pulses/mS 0b0001000000000000, // 1 pulses/mS 0b1000000100000000, // 2 pulses/mS 0b1000010000010000, // 3 pulses/mS 0b1000100010001000, // 4 pulses/mS 0b0100100100100100, // 5 pulses/mS 0b0010100101001010, // 6 pulses/mS 0b1010101010101010, // 7 pulses/mS };

uint16_t ui16_pwm_table_blue[LED_PWM_TABLE_LEN] = { 0b0000000000000000, // 0 pulses/mS 0b0000010000000100, // 2 pulses/mS 0b0100100010000100, // 4 pulses/mS 0b0100100101010010, // 6 pulses/mS 0b1010101010101010, // 8 pulses/mS 0b1010101110101011, // 10 pulses/mS 0b1110101110111110, // 12 pulses/mS 0b1110111110111111, // 14 pulses/mS };

rananna commented 3 years ago

I didn't know the nrf has two LEDs... or a reset button for that matter! Was looking to see the power spec of the RGB led - the APA102 leds I use have a max draw of 100mA so hoping it's less... :-o

yes the green one is a real power sink. The RGB is better,,,

4var1 commented 3 years ago

I didn't know the nrf has two LEDs... or a reset button for that matter! Was looking to see the power spec of the RGB led - the APA102 leds I use have a max draw of 100mA so hoping it's less... :-o

yes the green one is a real power sink. The RGB is better,,,

Interesting! Ok - am reworking some code. Can you look to see how you can call led_clock(); every 50mS in the remote please - then we'll save a decent amount of cycles in each interrupt.

That call deals with the sequence queue -it's not related to lighting the actual LED - but it does need to be on a regular 50mS clock.

rananna commented 3 years ago

ui16_pwm_table_blue

OK, avg current is about 2ma with all 1's (full on white) all_ones

rananna commented 3 years ago

I didn't know the nrf has two LEDs... or a reset button for that matter! Was looking to see the power spec of the RGB led - the APA102 leds I use have a max draw of 100mA so hoping it's less... :-o

yes the green one is a real power sink. The RGB is better,,,

Interesting! Ok - am reworking some code. Can you look to see how you can call led_clock(); every 50mS in the remote please - then we'll save a decent amount of cycles in each interrupt.

That call deals with the sequence queue -it's not related to lighting the actual LED - but it does need to be on a regular 50mS clock.

Why not just start a repeating 50ms app timer at the beginning of led_alert(), and shut it down when the alert finishes the sequence? that way it only consumes pwr during the alert.

rananna commented 3 years ago

And , your code becomes even more self contained, not relying on the timer in the wireless controller.

4var1 commented 3 years ago

ok - i assumed you were generally against timers :) I can look at that...

rananna commented 3 years ago

oops I am really referring to main_timer, not led_timer that is already in the common folder -I would like to see that one gone if possible as it is also consuming pwr.

rananna commented 3 years ago

No, timers are fine, they play well with the NORDIC PWR handling code. it is REPEATING timers that can create a big issue if we leave them running when they are not needed. That is why I was suggesting that you start and stop the timer on function entry/exit

rananna commented 3 years ago

also, app timers were used by the low power PWM approach of Nordic. We will just have to see what the power tradeoff if with pwm. Most people want pwm to control light intensity/uniformity and not to reduce power...

4var1 commented 3 years ago

@casainho just preparing a PR - for now I've not changed PWM frequency.as @rananna From your 100% on test - LED power is significant yes? - so doubling time on not something we should do first imho.

4var1 commented 3 years ago

ok - submitted. I did just realise I can go a step further and disable the sequence timer when there is no sequence. I'll work on that now.

4var1 commented 3 years ago

https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/86

rananna commented 3 years ago

ok - submitted. I did just realise I can go a step further and disable the sequence timer when there is no sequence. I'll work on that now.

And the need for main timer?

4var1 commented 3 years ago

@rananna - that is gone too! let's see how it performs :)