OpenSourceEBike / TSDZ2_wireless

TSDZ2_wireless
36 stars 11 forks source link

led bitstripes #89

Closed 4var1 closed 3 years ago

4var1 commented 3 years ago

test old bitstripes to compare power

4var1 commented 3 years ago

@rananna - this goes back to the old bitstripes where the LED is lit for shorter periods over the same duty cycle. Up to you if you think it's worth testing?

4var1 commented 3 years ago

I'd be interested to see - but totally understand if you're fed up of repeating the same tests :)

rananna commented 3 years ago

OK, here goes. First of all, I wrote a simple low power timer based delay routine to replace nrf_delay_ms. It works well, simply processing events until timeout. I put this in common.c:

timer

rananna commented 3 years ago

now the test code looks like this: I run the pwm code followed by simply turning on the RGB leds. brightness

casainho commented 3 years ago

Good! But I wounder if is a good idea to block, since that blocking time means the system will not process the buttons input, for instance.

rananna commented 3 years ago

But I wounder if is a good idea to block, since that blocking time means the system will not process the buttons input, for instance.

I don't think it blocks interrupts like button presses. sd_app_evt_wait () will process any pending interrupts, However, It will not process any code after the function call until the timer times out.

rananna commented 3 years ago

So, first of all with the old striping code. I trigger the brightness change with a double click of the button. here is the result: You can see in the trace a green flash caused by the double click followed by pwm R , G, and B then solid on R , G, B I repeat this three time in the trace for LOW, MED and HIGH brightness. old_striping

casainho commented 3 years ago

But I wounder if is a good idea to block, since that blocking time means the system will not process the buttons input, for instance.

I don't think it blocks interrupts like button presses. sd_app_evt_wait () will process any pending interrupts, However, It will not process any code after the function call until the timer times out.

What is strange to me is blocking waiting for the finish of the delay - I would expect to keep running to give opportunity for other parts of the code to run (state machine) and blocking only at the end on main loop with the sd_app_evt_wait().

rananna commented 3 years ago

Some current measurements: LOW: pwm R - 628uA pwm G-516ua pwm B-576ua solid R-979 ua solid G- 330uA solid B-706 uA

The trace is deceptive, there are high current spikes of short duration when the LEDS are on which adds a lot to the average current consumption. The green LED is also much more efficient!

rananna commented 3 years ago

MED: pwm R - 972uA! pwm G-583ua pwm B-835ua! solid R-984 ua solid G- 323ua solid B-710 uA

Big pwm jump in current on R and B for unknown reason.....

rananna commented 3 years ago

HIGH: pwm R - 1300 uA! pwm G-626 ua pwm B-1070 ua! solid R-991 ua solid G- 329 ua solid B-712 uA

again, big current spiking in pwm.

rananna commented 3 years ago

@casainho , @4var1 , If you are there, is there anything else you want me to test before moving on to the new striping technique?

rananna commented 3 years ago

BTW, The PWM has much improved uniformity in brightness over full on leds and the color mixing is really good

rananna commented 3 years ago

ok, moving on....

4var1 commented 3 years ago

Hi - no if you're happy with the power utilisation I'm good thanks

I am going to tidy up the led code a bit later this evening - only to combine some logic statements and make the code a bit more compact - but i think to get a comparison of the stripe types - we should test the code as-is.

On Fri, 19 Feb 2021 at 15:34, rananna notifications@github.com wrote:

ok, moving on....

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

rananna commented 3 years ago

wait, I'm not done! I am about to test the new striping ... that data was the old striping. stand by.....

casainho commented 3 years ago

Well, I think it is ok for the tests. In general we need to understand the battery estimation as we did before -- and I am curious on your simulation, if now the battery will have the same 2.5 years...

4var1 commented 3 years ago

wait, I'm not done! I am about to test the new striping ... that data was the old striping. stand by.....

ok no probs!

rananna commented 3 years ago

ok here is the trace with new striping. bigger current spike on Red for some reason: new striping

rananna commented 3 years ago

now for average current measurements: LOW: pwm R - 662 uA pwm G-520 ua pwm B-593 ua solid R-985 ua solid G- 334 uA solid B-707 uA

similar results to last time...... but current spikes are hard on the battery, maybe favoring old striping....

4var1 commented 3 years ago

ok - i'm easy either way - you choose - is a simple change to roll back to the single varying-length pulses.

rananna commented 3 years ago

MED: pwm R - 1008 ma! pwm G-585 ua pwm B-848 ua! solid R-988 ua solid G- 332 ua solid B-706 uA

again similar to old striping

rananna commented 3 years ago

HIGH: pwm R - 1377 uA! pwm G-660 ua pwm B-1075 ua! solid R-984 ua solid G- 322 ua solid B-711 uA

4var1 commented 3 years ago

The variations in power utilisation are odd. I've never really done a study on led power draw so can't say if this is unexpected behaviour or not - but you'd think that if the duty cycle increased by a fixed amount - then so would the power utilised...

Each step of brightness should add 12.5% duty cycle for red/blue - and 6.25% for green... using either stripe method

rananna commented 3 years ago

ok done. What to make of this?

  1. there is probably a small advantage to the old striping technique as current spiking is less

  2. pwm is not a huge power saver. Comparing low pwm to full on LEDs: RED: pwm is 67% of the full on power GREEN: pwm consume 55% MORE power than full on led BLUE: pwm is 87% of the full on power

  3. power difference for low med compared to pwm high brightness: RED low: 48% red MED: 73% green low: 78% green med: 88% blue low: 55% blue med: 78%

@4var1 , There is a power saving on LOW., Medium (step 4) should be reduced a little bit more in brightness

rananna commented 3 years ago

overall conclusion: PWM is giving us a modest power saving compared to full on leds for red and blue, even on low brightness. No savings at all for the green LED., it does have other advantages however as cited above. The processor running during pwm is probably the cause of higher current.

rananna commented 3 years ago

ok that is it for me. @4var1 , @casainho : what are youyr thoughts on this?

4var1 commented 3 years ago

I cant' see how green can use more power... If anything that should be the most efficient LED because it's so bright - so we underdrive it...

4var1 commented 3 years ago

I'll be more engaged a bit later - still at work - meetings last thing on a Friday - who books those :)

casainho commented 3 years ago

The variations in power utilisation are odd. I've never really done a study on led power draw so can't say if this is unexpected behaviour or not - but you'd think that if the duty cycle increased by a fixed amount - then so would the power utilised...

Each step of brightness should add 12.5% duty cycle for red/blue - and 6.25% for green... using either stripe method

With the duty_cycle we are applying a percentage of fixed battery voltage to LED but the LED current does not increase linearly with the voltage and so the power also does not:

image

@rananna for me is ok, I just wish to know for how long the battery estimation will be now - the same 2.5 years??

rananna commented 3 years ago

@rananna for me is ok, I just wish to know for how long the battery estimation will be now - the same 2.5 years??

@casainho , are you planning to do a test on a bike this weekend?

My plan was to clean up any bugs you may find from testing, we would release a new version, and then do a complete power analysis on that new release.

casainho commented 3 years ago

I may have an opportunity to ride on Sunday - I will test it on Sunday afternoon.

4var1 commented 3 years ago

Alas it's a wet weekend here... so probably no biking for me. I did just buy a fsr frame on ebay though... so a use for my second motor - and i'll put a remote on that build.

I'll finish tidying things up tomorrow by mid-morning hopefully - i would like to change the api slightly for ledalerts - but will mean a quick search and replace so don't worry! The separate hold calls are a but untidy so will replace them with single calls to either queue/play now etc.

On Fri, 19 Feb 2021 at 20:10, casainho notifications@github.com wrote:

I may have an opportunity to ride on Sunday - I will test it on Sunday afternoon.

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

4var1 commented 3 years ago

I'm also implementing street mode as it's a feature I think we should keep

@casainho - one thing i always found with street mode as I prefer configure my bike to power up in street mode and then switch off if needed (so I can claim it's street legal if challenged) - is that it's very easy to enable walk assist instead if you don't hold the key combo properly (hotkey is hold down+onoff)... and if you do that and you're not expecting the bike to lurch forward could cause blue gear damage. Is it too much to suggest a change to up+onoff hold to toggle street mode? Then the worst you can do is either switch off - or toggle the lights!

On Fri, 19 Feb 2021 at 22:10, Ben Machin ben.machin@gmail.com wrote:

Alas it's a wet weekend here... so probably no biking for me. I did just buy a fsr frame on ebay though... so a use for my second motor - and i'll put a remote on that build.

I'll finish tidying things up tomorrow by mid-morning hopefully - i would like to change the api slightly for ledalerts - but will mean a quick search and replace so don't worry! The separate hold calls are a but untidy so will replace them with single calls to either queue/play now etc.

On Fri, 19 Feb 2021 at 20:10, casainho notifications@github.com wrote:

I may have an opportunity to ride on Sunday - I will test it on Sunday afternoon.

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

4var1 commented 3 years ago

@casainho - i'm having trouble getting the street mode settings to persist

rananna commented 3 years ago

Have not implemented in the wireless controller. IMO street mode is a non issue in North America. There is no effective policing of e-bike motor wattages or speeds on the bike pathways. People are putting fake stickers on their motors, but nobody is inspecting.

I know the situation is different in Europe, and it will probably change here, but for now it is a non-issue.

4var1 commented 3 years ago

@casainho - whatever I have set in the app - this is never true on the wireless controller - neither variable ever get set to true...

(ui_vars.ui8_street_mode_hotkey_enabled && ui_vars.ui8_street_mode_function_enabled)

4var1 commented 3 years ago

@casainho Mmmm... looks like an issue in the wireless controller - the code below cannot be good, sets street_mode_enabled twice from two different packet bytes - overwriting the original value. I notice in the app there are two different config packet versions - but seems the wireless controller assumes a mix! :)

Edit - oh no the app is the same - has two bytes for the same field.. and as bad as that looks - all the code relating to tx/rx packets seem to match up so shouldn't cause a problem... since they all should have the same value... but something isn't working.

ui_vars.ui8_street_mode_function_enabled = data[148];
**ui_vars.ui8_street_mode_enabled = data[149];**
ui_vars.ui8_street_mode_enabled_on_startup = data[150];
ui_vars.ui8_street_mode_speed_limit = data[151];
ui_vars.ui8_street_mode_power_limit_div25 = data[152];
ui_vars.ui8_street_mode_throttle_enabled = data[153];
ui_vars.ui8_street_mode_hotkey_enabled = data[154];
ui_vars.ui8_pedal_cadence_fast_stop = data[155];
ui_vars.ui8_throttle_virtual_step = data[156];
**ui_vars.ui8_street_mode_enabled = data[157];**
4var1 commented 3 years ago

@casainho Mmmm... looks like an issue in the wireless controller - the code below cannot be good, sets street_mode_enabled twice from two different packet bytes - overwriting the original value. I notice in the app there are two different config packet versions - but seems the wireless controller assumes a mix! :)

Edit - oh no the app is the same - has two bytes for the same field.. and as bad as that looks - all the code relating to tx/rx packets seem to match up so shouldn't cause a problem... since they all should have the same value... but something isn't working.

ui_vars.ui8_street_mode_function_enabled = data[148];
**ui_vars.ui8_street_mode_enabled = data[149];**
ui_vars.ui8_street_mode_enabled_on_startup = data[150];
ui_vars.ui8_street_mode_speed_limit = data[151];
ui_vars.ui8_street_mode_power_limit_div25 = data[152];
ui_vars.ui8_street_mode_throttle_enabled = data[153];
ui_vars.ui8_street_mode_hotkey_enabled = data[154];
ui_vars.ui8_pedal_cadence_fast_stop = data[155];
ui_vars.ui8_throttle_virtual_step = data[156];
**ui_vars.ui8_street_mode_enabled = data[157];**

ui8_street_mode_hotkey_enabled doesn't appear to be set by the app at all - is it still required? Seems a redundant setting...

I guess you might want it if you didn't want the bike to be able to be taken out of its startup setting...

casainho commented 3 years ago

Street mode is not a priority for the next version.

4var1 commented 3 years ago

Street mode is not a priority for the next version.

Oh... I've spent half the afternoon sorting it out - and the wired remote code all works - it's just the enable flag that isn't coming from the app. I was just about to say the app doesn't remember the street mode power limit either...

If that isn't working - then hiding it from the config page will prevent a lot of questions - and confusion... but we could just try to get it working :)

casainho commented 3 years ago

Well, then I will need to hide on the app. I do not have time to develop that feature and test and that is an advanced feature that original firmware does not have - the focus here is the wireless and not that advanced features. With time we will be able to implement them back.

4var1 commented 3 years ago

PR submitted https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/93