OpenSourceEBike / TSDZ2_wireless

TSDZ2_wireless
35 stars 11 forks source link

Update ledalert.c #87

Closed 4var1 closed 3 years ago

4var1 commented 3 years ago

more power improvements - hopefully should improve power utilisation when led is on - and show differences in power utilisation between brightness levels...

rananna commented 3 years ago

unfortunately, the three states are still the same. here is the trace with low,med,high brightness. three cycles, each with R,G,B during each cycle 3 states

will try @casainho 's idea of pwm on an empty pin next.

rananna commented 3 years ago

ok, I modified pca10059.h as follows: //#define LED2_R NRF_GPIO_PIN_MAP(0,8) //#define LED2_G NRF_GPIO_PIN_MAP(1,9) //#define LED2_B NRF_GPIO_PIN_MAP(0,12)

define LED2_R NRF_GPIO_PIN_MAP(0,2)

define LED2_G NRF_GPIO_PIN_MAP(1,3)

define LED2_B NRF_GPIO_PIN_MAP(0,4)

so that the led mapping goes to an empty pin.

re-compiled and ran again. Of course, no leds light. here is the result when toggling the pwm brightness through the 3 levels:

pwm_noleds

I am now very confused. the power dissipation has virtually NOTHING to do with LEDs. ?????

4var1 commented 3 years ago

Oh that's disappointing! I'll double check my code - but now it should be switching the pin far less - and never asserting when the pin is already in the correct state....

So the cost all appears to be in the app_timer - and not related to how fast it's running... Makes no sense...

More investigation needed - will take another look after work!

rananna commented 3 years ago

more weirdness. here is the standby current in the idle loop, about 75uA on average. button_released

Now, if you hold down a button on the remote (which has no action associated with it) the power dissipation increases to 344uA! button_pressed

maybe it is briefly restarting in the idle loop leading to the higher dissipation ?? Is the pressed state of the pin dissipating power through the internal resistor? don't know why this is happening.....

Finally, here is the current profile obtained by pressing (and holding) the plus key with the motor off. In this case, I do a brief red flash which you can see in the trace:

button_pressed -red flash

The current during the flash is about 866 uA, which is reasonable.

button_pressed -red flash_no hold

If you dont hold the plus key down (do a quick click) the current drops to 600 uA for the LED

rananna commented 3 years ago

Why does this single Red LED flash have so much less current? is the higher current related to the sequencing?

rananna commented 3 years ago

ok, so now I have moved on to measure just single flashes. The SOC display for the motor is good for this. It flashes 10X with green LED, as my battery is fully charged: here are the three states along with the LED current measurements:

Soc low intensity: SOC_dim

SOC Medium Intensity: SOC_med

SOC high intensity: SOC_bright

the LED current is 500 ua, 550uA, and 610 uA respectively. PWM seems to be working well in this case.

rananna commented 3 years ago

so back on sequencing, here is the trace with 5 red leds followed by 5 green leds: 5 red leds followed by 5 green leds

here is 10 blue leds: 10 blue leds

back to the excessive power dissipation.

rananna commented 3 years ago

my idle loop is: ` while (true) {

// Wait for an event.
__WFE();
// Clear the internal event register.
__SEV();
__WFE();
check_interrupt_flags();

} } is the sequencer forcing it out out of idle? if so, that would certainly cause a big power dissipation .

rananna commented 3 years ago

ok, so what if I did this: ` while (true)

{

/ // Wait for an event. WFE(); // Clear the internal event register. SEV(); __WFE(); check_interrupt_flags(); / }`

no idling at all! here is the result: no_idle

Note that there is a similar current level to what we were seeing in sequencing.

casainho commented 3 years ago

But even that 8mA seems to much for not LED... can that be cause of Bluetooth? or ANT? -- can you try to disable all that things and see how much the NRF52 use on the continuous while loop?

rananna commented 3 years ago

@casainho , ok, with the following code: int main(void) { while (true) {} }

you get the following: no_code

So, the chip does consume a fair bit of power.

4var1 commented 3 years ago

@rananna @casainho - so what's the current (no pun intended!) thinking - it's the 50mS timer for the sequencer that's causing the nrf to shift out of a low power mode? Am just finishing work - will be able to look at code in a couple of hours if there are changes/suggestions you have?

4var1 commented 3 years ago

Ah didn't see your last post - the left axis is cut off - is that it using c.8mA doing nada?

4var1 commented 3 years ago

@rananna did you see this quote from nordic:

The chip will enter emulated system OFF if it is in debug interface mode. This may also explain the high current consumption as it will make sd_power_system_off() return with an error (should never return during normal program execution when no debugger is attached). 7mA matches the CPU run current when the LDO regulator is used.

From https://devzone.nordicsemi.com/f/nordic-q-a/50306/system-not-going-to-sleep-and-or-taking-too-much-current

rananna commented 3 years ago

sorry , here you go. no_code

4var1 commented 3 years ago

@rananna I assume you've read this - but if not looks useful

https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/optimizing-power-on-nrf52-designs

seems 7mA is what the cpu draws... sorry 4mA

@rananna I assume you've read this - but if not looks useful https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/optimizing-power-on-nrf52-designs seems 7mA is what the cpu draws...

yup, knew about that one as well. Looks like you are going down the same path I was on a few months ago. :)

sure - but it says 4mA when CPU is running - and other articles quote 7mA - so what we're seeing is expected no?

rananna commented 3 years ago

The chip will enter emulated system OFF if it is in debug interface mode. This may also explain the high current consumption as it will make sd_power_system_off() return with an error (should never return during normal program execution when no debugger is attached). 7mA matches the CPU run current when the LDO regulator is used.

From https://devzone.nordicsemi.com/f/nordic-q-a/50306/system-not-going-to-sleep-and-or-taking-too-much-current

yeah, I am aware of this one. I always unplug from the debugger and power cycle before doing measurements.

rananna commented 3 years ago

@rananna I assume you've read this - but if not looks useful

https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/optimizing-power-on-nrf52-designs

seems 7mA is what the cpu draws...

yup, knew about that one as well. Looks like you are going down the same path I was on a few months ago. :)

4var1 commented 3 years ago

The chip will enter emulated system OFF if it is in debug interface mode. This may also explain the high current consumption as it will make sd_power_system_off() return with an error (should never return during normal program execution when no debugger is attached). 7mA matches the CPU run current when the LDO regulator is used. From https://devzone.nordicsemi.com/f/nordic-q-a/50306/system-not-going-to-sleep-and-or-taking-too-much-current

yeah, I am aware of this one. I always unplug from the debugger and power cycle before doing measurements.

the point was the last sentence - 7mA matches the CPU run current when the LDO regulator is used.

And the power article says 4mA - so what's the issue - or am I missing the point massively? :)

4var1 commented 3 years ago

@rananna ok the cpu should be sleeping between sequence interrupts - that's the issue yea? Do I need to call something at the end of the sequence ISR to put it to sleep until the next interrupt?

sd_app_evt_wait() ?

but hang on - chances are if the sequence interrupt is running - then so is the pwm interrupt - so it's not going to sleep.... I only enable the sequence interrupt during a sequence...

So what am I missing - how can you expect the cpu to sleep (and use less than 4-5mA) when it's doing stuff? :)

If it's not sleeping when the sequeunce is done - shall I try calling sd_app_evt_wait() after I disable the timers?

rananna commented 3 years ago

Is it possible to increase the sequence timer interval?

4var1 commented 3 years ago

Is it possible to increase the sequence timer interval?

yes - if we increase the minimum LED flash length. 50mS means all led flashes and gaps have to be multiples of 50mS etc.

So I know what we're doing and don't keep asking stupid questions - what are we trying to fix? Why should the cpu not use 4mA when running the two timers and ISRs?

4var1 commented 3 years ago

Ok - I'll leave you to it.

https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/88 in case it's useful.

casainho commented 3 years ago

I think the idea for low power is to use interrupts. When they fire, we just do our processing and then we enter low power again. To enter in low power, usually is done at the end of main loop - this also means that after interrupt fire, the main loop will run once untill the enter low power instructions are executed. For the PWM low power, an APP TIMER is used because it is like interrupt firing at each change if the signal, so twice the PWM frequency and all other time system will be in low power. APP TIMER uses the RTC peripherial that is like a programmable timer that uses very low power.

rananna commented 3 years ago

Well, I guess it depends on what we consider the issue to be. If we wanted a flexible signalling approach with dimming, we have that working well. If we are also trying add the requirement of minimal power consumption to the spec it might be possible that we are not there yet. For example, if we simply use full power dc flashing without pwm and the need for two timers, maybe the overall power consumption during signalling would be less than our current pwm approach, as the LEDs seem to be consuming about a ma when running, compared to the 7ma we are seeing due to CPU activity. On the other hand, I really need to do a new battery lifetime estimation. Led signalling is not a large portion of the power load.

rananna commented 3 years ago

And before we lose sight of this, as posted above the single flashing sequences work well. It isn't obvious to me why that is the case. However, It would not be too difficult to modify the signalling in the remote to only use a single color at a time.

rananna commented 3 years ago

So, I have to go out for a couple of hours, but when I get back I will play with the sequencing a bit more. Stand by.......

casainho commented 3 years ago

You could try to isolate the code run for a pin without anything connected. You should measure the very low power current if the NRF52 sets the pin but stays always at low power. Then add the PWM and see the difference. Since at full time running the NRF52 uses the 7 mA, the. I would hope to use no more than 5% processing time to implement the PWM, so, 7*0.05 = 350uA.

4var1 commented 3 years ago

Looks like app_timers are treated slightly differently depending on what sets them up - see below - sequencer timer is first type - pwm timer 2nd type - think, not 100% sure - since they are both created from a call from main - but one is started by a call from main - the other is started by a call in the first interrupt. I'm looking into this more to see if it has any relevance.

apptimer called from main context (or interrupt priority lower than for SWI0). Interrupt is triggered, and the timer operation is processed immediately before continuing with the main thread. apptimer called from interrupt context of same or higher priority as the SWI0 used by the apptimer. In that case the interrupt is triggered, but the existing interrupt routine (and any other with higher priority) will finish before the app_timer interrupt routine is run.

4var1 commented 3 years ago

Hi @rananna - why do you still call nrf_delay_ms after queueing led alerts? Not saying it's related to the power issue - but wondered if it's still necessary to block after calling?

Was also surprised to see in the remote code for brake signalling - isn't that your most hated timer? :)

ui32_time_now = get_time_base_counter_1ms();
rananna commented 3 years ago

I'll check again when I get back, but I thought any delay was before calling. If it is after it would be a holdover from previous code and I will remove.

4var1 commented 3 years ago

NRF_PWR_MGMT_CONFIG_FPU_SUPPORT_ENABLED = 1 is enabled in the wireless controller for some reason. I can't find any use of the FPU in the remote (although you did have floats i recall for leds before but I can't find any now) - I've read that not clearing FPU interrupt flags can stop sd_app_evt_wait from putting the cpu to sleep....

4var1 commented 3 years ago

Right - I'm going to stop spamming random links now and go and have a beer and watch a different screen for a bit :)

But this is recent - and with v16 SDK - so might be more relevant - https://devzone.nordicsemi.com/f/nordic-q-a/64035/nrf_pwr_mgmt_run-with-softdevice-not-go-to-low-power/261785

4var1 commented 3 years ago

Oh you do enable FPU in the makefile - just thought to look there - should remove that I think?

CFLAGS += -mfloat-abi=hard -mfpu=fpv4-sp-d16

casainho commented 3 years ago

To know if I was in low power, I added a pin "blink" on the main loop and then I measured with oscilloscope, to understand if there was a continuous blink and that would mean no low power, as low power blocks until next wake-up. You guys can make an LED blink on the main loop to visually see if the system is in low power and you should turn off the LED before enter low power and turn on the LED just after the instructions for the enter low power.

4var1 commented 3 years ago

To know if I was in low power, I added a pin "blink" on the main loop and then I measured with oscilloscope, to understand if there was a continuous blink and that would mean no low power, as low power blocks until next wake-up. You guys can make an LED blink on the main loop to visually see if the system is in low power and you should turn off the LED before enter low power and turn on the LED just after the instructions for the enter low power.

You don't even need to do the blink - you can set NRF_PWR_MGMT_CONFIG_DEBUG_PIN_ENABLED and it will light a LED when the CPU is in low power

4var1 commented 3 years ago

To know if I was in low power, I added a pin "blink" on the main loop and then I measured with oscilloscope, to understand if there was a continuous blink and that would mean no low power, as low power blocks until next wake-up. You guys can make an LED blink on the main loop to visually see if the system is in low power and you should turn off the LED before enter low power and turn on the LED just after the instructions for the enter low power.

You don't even need to do the blink - you can set NRF_PWR_MGMT_CONFIG_DEBUG_PIN_ENABLED and it will light a LED when the CPU is in low power

Unfortunately an oscilloscope is one of the few things I don't have... although the frequencies we're talking about I could probably knock something up with an FT232 :)

rananna commented 3 years ago

i DI

You don't even need to do the blink - you can set NRF_PWR_MGMT_CONFIG_DEBUG_PIN_ENABLED and it will light a LED when the CPU is in low power

That's what I did to confirm low power mode.

rananna commented 3 years ago

Oh you do enable FPU in the makefile - just thought to look there - should remove that I think?

CFLAGS += -mfloat-abi=hard -mfpu=fpv4-sp-d16

Thanks - from older code - removed. (there is no effect on entering low power mode , I was already entering low power mode.)

rananna commented 3 years ago

Hi @rananna - why do you still call nrf_delay_ms after queueing led alerts? Not saying it's related to the power issue - but wondered if it's still necessary to block after calling?

Was also surprised to see in the remote code for brake signalling - isn't that your most hated timer? :)

ui32_time_now = get_time_base_counter_1ms();

@4var1 , I'm sorry for putting you to work today on the power issue. my bad, it was my code all along! The nrf_delay_ms function blocks entry into low power, and keeps the cpu at highest power consumption!!! I remove all delays for the LED sequencing and now we don't see the high power consumption. So, here is a RED, GREEN BLUE GREEN sequence for low, medium and high intensity leds. GREEN 420 GREEN-478 GREEN-511 The good news is that current is now MUCH lower. However, there are spikes in the current where the processor leaves low power briefly. Not sure where that is coming from. As you can see from the trace GREEN low intensity current is ~420ms, medium is ~478 ma and High is 511ma. There is obviously not a big difference is current consumption between the three. For comparision, however, a full on Green led consumes about 1.1 ma: Green-fullon

You can see the effect os a nrf_delay_ms(5000) in this chart - see the high current spike caused by not sleeping!

Again , sorry for not noticing this previously.....

4var1 commented 3 years ago

Ah no worries, nice one - very glad we've got to the bottom of it! :)

4var1 commented 3 years ago

Question now is - was my code better before i fiddled with it - or after...

4var1 commented 3 years ago

@rananna The main changes today were - I stopped asserting pins when the state hadn't changed - so didn't pump 0's in succession etc. that I think is a good change we should keep.

But the other was I changed the bitstripes for intensities so that rather than playing lots of short flashes with varying gaps - I moved to doing the same duty cycle - but as a single run of 1s of varying length per intensity. I did this to reduce the number of switches between 0/1 - as we thought that might be the cause of current draw.

Do you think that a run of 1's give it a chance to peak at a higher current draw - we're talking 61uS per 1...

Should we test the old staccato bitstripes?

rananna commented 3 years ago

Yes, we should test the old bit stripes. Could you please push a PR for this?

4var1 commented 3 years ago

yep - done! https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/89

4var1 commented 3 years ago

@rananna - once you've had a chance to test - let me know which method we should keep - then tonight I'll have a bit of a code tidy - get ready (hopefully) for the start of release testing... :)

On Thu, 18 Feb 2021 at 21:29, rananna notifications@github.com wrote:

Yes, we should test the old bit stripes. Could you please push a PR for this?

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