search

OpenSourceEBike / TSDZ2_wireless

TSDZ2_wireless
35 stars 11 forks source link

PWM signalling #80

Closed rananna closed 3 years ago

rananna commented 3 years ago

@casinho,

I have incorporated @4var1's pwm technique, and it is a MUCH better approach than I was using. It is very interrupt friendly, and as a result, the assist increase/decrease button response is MUCH better.

The signalling is now consistent, and I have update the remote operation docs to reflect this. Please accept the PR for opensource.io as well, as it goes with this PR

I will now do a power study on the remote with this new signalling approach to estimate battery lifetime.

@4var1 : per @ casinho's request, I have added the led signalling code to the common firmware directory. You will note that I added quite a few new signalling defines. You might want to update the motor firmware to reference this directory. Also, I added a double press off the enter key to change led intensity using the new function you added, but I really don't see much visible change. Could you please look at this? As mentioned above, I need to do a power analysis to really see the power changes at each of the 3 levels.

4var1 commented 3 years ago

Hi, am away from my pc at the moment but there are now 7 levels of brightness but the comments still say only 3. Try using brightness 1,4,7 as your three levels you should see a difference. Cheers, Ben

On Sun, 14 Feb 2021, 15:40 rananna, notifications@github.com wrote:

@casinho,

I have incorporated @4var1 https://github.com/4var1's pwm technique, and it is a MUCH better approach than I was using. It is very interrupt friendly, and as a result, the assist increase/decrease button response is MUCH better.

The signalling is now consistent, and I have update the remote operation docs to reflect this. Please accept the PR for opensource.io as well, as it goes with this PR

I will now do a power study on the remote with this new signalling approach to estimate battery lifetime.

@4var1 https://github.com/4var1 : per @ casinho's request, I have added the led signalling code to the common firmware directory. You will note that I added quite a few new signalling defines. You might want to update the motor firmware to reference this directory. Also, I added a double press off the enter key to change led intensity using the new function you added, but I really don't see much visible change. Could you please look at this? As mentioned above, I need to do a power analysis to really see the power changes at each of the 3 levels.

You can view, comment on, or merge this pull request online at:

https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/80 Commit Summary

  • new led pwm
  • update docs and cleanup
  • update docs
  • Merge branch 'master' of https://github.com/rananna/TSDZ2_wireless
  • Merge pull request #1 from OpenSourceEBike/master
  • led signalling
  • Merge pull request #2 from OpenSourceEBike/master
  • led signalling
  • signally changes
  • delete
  • ledalert
  • Merge pull request #3 from OpenSourceEBike/master
  • led signals
  • Merge pull request #4 from OpenSourceEBike/master
  • led notification
  • led alerts
  • intensity

File Changes

Patch Links:

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

rananna commented 3 years ago

@4var1 , I tried to incorporate 1, 4, and 7 intensities into the code, but 1 and 7 look the same, while 4 has the leds completely off! Maybe I messed up this part of the code in the common directory version. @casainho has accepted the commit, could you please merge and have a look at the code in the common directory?

4var1 commented 3 years ago

I just looked at the code you put in common - looks ok on first scan. I'll update my repo and point the wireless controller at common_firmware - see if I get any oddities.

rananna commented 3 years ago

please check intensity levels as well.

4var1 commented 3 years ago

yes that's what I'm checking - is there something else?

4var1 commented 3 years ago

Do we have to move disp_soc() into led_alerts? Makes my nice generic library not so generic :)

4var1 commented 3 years ago

The three red flashes when you've hit assist level limits is too long - if you press down twice at limit then it takes a while to play out the 6 flashes - so you then see your up presses with a lag. I think it needs to go back to a single flash that's the same length as the up/down flashes.

4var1 commented 3 years ago

but yes, there does seem to be a problem with brightness 4.... investigating!

4var1 commented 3 years ago

Ok - i've found the problem - sorry about that - there's now much more of a different between 7 and 1 too! :)

Sorry to be picky about the leds really not trying to be awkward - but I also wonder why you changed the motor on start sequence flash - as the led is on much more now (and so uses more power) than it was before since you reduced the time off in between the yellow flashes - was that intentional? Also the yellow flash now doesn't match the cadence of the follow-on 'motor on complete' green flash... so it doesn't look as slick in my opinion :)

4var1 commented 3 years ago

Hi - for my understanding - what's the reason for adding the below declaration in ledalerts.h?

extern uint32_t get_time_base_counter_1ms(void);

rananna commented 3 years ago

Do we have to move disp_soc() into led_alerts? Makes my nice generic library not so generic :)

Well, I had the opposite thought, this library is all about led signalling for the TSDZ2 motor, so I moved the SOC signally to be with it......

rananna commented 3 years ago

a lag. I think it needs to go back to a single flash that's the same length as the up/down flashes.

I kill the queue so it only plays once. Seems fast to me with only one play. Any way no problem, one blink or three it is all the same to me......

rananna commented 3 years ago

Ok - i've found the problem - sorry about that - there's now much more of a different between 7 and 1 too! :)

Sorry to be picky about the leds really not trying to be awkward - but I also wonder why you changed the motor on start sequence flash - as the led is on much more now (and so uses more power) than it was before since you reduced the time off in between the yellow flashes - was that intentional? Also the yellow flash now doesn't match the cadence of the follow-on 'motor on complete' green flash... so it doesn't look as slick in my opinion :)

No problem- I have been busy with just getting everything working and have not been focussed on aesthetics.

rananna commented 3 years ago

Hi - for my understanding - what's the reason for adding the below declaration in ledalerts.h?

extern uint32_t get_time_base_counter_1ms(void);

I needed to call this function from the wireless remote main and wanted to eliminate the intellisense error!

rananna commented 3 years ago

where did you put disp_soc? I see the old one in common.c

4var1 commented 3 years ago

Oh sorry I didn't put it anywhere yet - you'd need to put it in your code for now. I only noticed that you'd moved it as I got a compile error as it's in the wireless controller main.c for me.

My reasoning around disp_soc() is that I might publish the library as a separate project - as it's useful for anyone who wants to play LED sequences on an nrf or similar controller. If we put disp_soc() in there - then I can't keep the same codebase....

On the external - so that's not really needed? That's just to get around vscode seemingly not being able to find includes despite the paths looking ok in the json files? :)

On Sun, 14 Feb 2021 at 21:54, rananna notifications@github.com wrote:

where did you put disp_soc? I see the old one in common.c

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

4var1 commented 3 years ago

I'll put it in common.c - didn't realise that existed!

On Sun, 14 Feb 2021 at 22:27, Ben Machin ben.machin@gmail.com wrote:

Oh sorry I didn't put it anywhere yet - you'd need to put it in your code for now. I only noticed that you'd moved it as I got a compile error as it's in the wireless controller main.c for me.

My reasoning around disp_soc() is that I might publish the library as a separate project - as it's useful for anyone who wants to play LED sequences on an nrf or similar controller. If we put disp_soc() in there - then I can't keep the same codebase....

On the external - so that's not really needed? That's just to get around vscode seemingly not being able to find includes despite the paths looking ok in the json files? :)

On Sun, 14 Feb 2021 at 21:54, rananna notifications@github.com wrote:

where did you put disp_soc? I see the old one in common.c

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

rananna commented 3 years ago

Oh sorry I didn't put it anywhere yet - you'd need to put it in your code for now. I only noticed that you'd moved it as I got a compile error as it's in the wireless controller main.c for me. My reasoning around disp_soc() is that I might publish the library as a separate project - as it's useful for anyone who wants to play LED sequences on an nrf or similar controller. If we put disp_soc() in there - then I can't keep the same codebase.... On the external - so that's not really needed? That's just to get around vscode seemingly not being able to find includes despite the paths looking ok in the json files? :) … On Sun, 14 Feb 2021 at 21:54, rananna @.***> wrote: where did you put disp_soc? I see the old one in common.c — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASRDIVVN53KIWEB4PWHRR2TS7BBARANCNFSM4XTKXDJA .

No problem. I added it to common.c You are right about the external, I was being a bit lazy :)