OpenSourceEBike / TSDZ2_wireless

TSDZ2_wireless
36 stars 11 forks source link

Update main.c #84

Closed 4var1 closed 3 years ago

4var1 commented 3 years ago

Hopefully should fix the issue with walk assist timeout when activated from the remote.

4var1 commented 3 years ago

I think there might be undefined behaviour if you have both wired and wireless buttons attached - and activate walk assist from both concurrently... do we need to cater for that - it's quite an edge case :)

4var1 commented 3 years ago

@casainho - now we have LEDs for feedback - is it worth looking again at 'virtual throttle' before we get testing for a release?

Are you also ok if I do a tidy up of the code in main.c for the wireless controller. A lot of the code for button handling is spread across various functions that are named for screen type reasons. Needs a tidy up.

rananna commented 3 years ago

I think there might be undefined behaviour if you have both wired and wireless buttons attached - and activate walk assist from both concurrently... do we need to cater for that - it's quite an edge case :)

I was just playing with that case. mostly assist levels get confused, but no bad things happen. IMO I don't think we need worry about it though. The users will have one or the other remote.

4var1 commented 3 years ago

Oh the main interaction would be that the remote overrides whatever the wired buttons does - but that's fine - won't cause problems.

4var1 commented 3 years ago

Oh you have wired buttons too? I should really make a remote...

rananna commented 3 years ago

Yup, I wanted to make sure there were no issues if you happened to have both. However the wired version is a lot simpler to make compared to thee wireless version! :) Please give it a try -I would appreciate any feedback.

4var1 commented 3 years ago

I have all the bits so will do. I'm trying to find a reasonably priced FSR to convert with the spare motor I have - so will probably use the remote for that. I might run 5v for it though - we'll see how the power stuff works out :)

rananna commented 3 years ago

With the nordic dongle, 5v means using the first power conversion stage. A little higher power dissipation on battery.

On that topic, I had some fun with Nordic re: power consumption. See: https://devzone.nordicsemi.com/f/nordic-q-a/70031/nordic-dongle-power-off-mode/287680#287680

4var1 commented 3 years ago

With the nordic dongle, 5v means using the first power conversion stage. A little higher power dissipation on battery.

On that topic, I had some fun with Nordic re: power consumption. See: https://devzone.nordicsemi.com/f/nordic-q-a/70031/nordic-dongle-power-off-mode/287680#287680

So you've got it right down - looks like a useful drop for sure, nice work! I'm really interested to see what impact my PWM ISR running at 16khz has, hopefully doesn't suck power when the remote is on undoing all your good when-powered-off work! :)

4var1 commented 3 years ago

With the nordic dongle, 5v means using the first power conversion stage. A little higher power dissipation on battery.

On that topic, I had some fun with Nordic re: power consumption. See: https://devzone.nordicsemi.com/f/nordic-q-a/70031/nordic-dongle-power-off-mode/287680#287680

But do I have 3.3v available elsewhere? OTherwise I either need a battery or a DC-DC converter? Oh I could take 3.3v from the wireless controller! :)

4var1 commented 3 years ago

With the nordic dongle, 5v means using the first power conversion stage. A little higher power dissipation on battery. On that topic, I had some fun with Nordic re: power consumption. See: https://devzone.nordicsemi.com/f/nordic-q-a/70031/nordic-dongle-power-off-mode/287680#287680

But do I have 3.3v available elsewhere? OTherwise I either need a battery or a DC-DC converter? Oh I could take 3.3v from the wireless controller! :)

Oh I see sorry - I meant i would have a remote but run it from 5v from the bike battery - not a button cell or other supply. I don't like the idea of a remote battery - just something else to go wrong - and in a landfill afterwards!

Edit - ok i don't throw batteries in general waste before you tell me off! but it's still something I'll probably power from the bike battery just for reliability. And I'll probably have 5v on the handlebars for USB charging...

4var1 commented 3 years ago

@rananna - I was thinking about the issue we had with the walk assist. Really the remote should be sending button state to the wireless controller - and that then handles all the feature state.

That would remove all the conflicts - and would mean you don't need to duplicate the state handling code...

rananna commented 3 years ago

You are probably right, this would certainly simplify the code. If I had known a few months ago that we would be implementing button control in the wireless controller, the design of the wireless remote would gave been very different.

However, as it is now functioning well, I don't want to make a fairly major change to the software architecture at this late stage. Also, the only real conflict was walk assist, every other function of the wireless remote plays well with the wired remote. (With the exception of assist changing which can get confusing)

Maybe we should add this as an issue in GitHub for possible future redesign.

casainho commented 3 years ago

@rananna - I was thinking about the issue we had with the walk assist. Really the remote should be sending button state to the wireless controller - and that then handles all the feature state.

That would remove all the conflicts - and would mean you don't need to duplicate the state handling code...

That is maybe a good idea, like if the wireless remote or wired remote are just buttons, making the wireless remote more dumb. But then there is also the LEDs... maybe this is a bit hard to implement...

4var1 commented 3 years ago

You are probably right, this would certainly simplify the code. If I had known a few months ago that we would be implementing button control in the wireless controller, the design of the wireless remote would gave been very different.

However, as it is now functioning well, I don't want to make a fairly major change to the software architecture at this late stage. Also, the only real conflict was walk assist, every other function of the wireless remote plays well with the wired remote. (With the exception of assist changing which can get confusing)

Maybe we should add this as an issue in GitHub for possible future redesign.

Aren't you in Canada - shouldn't you still be asleep? :)

@rananna - I was thinking about the issue we had with the walk assist. Really the remote should be sending button state to the wireless controller - and that then handles all the feature state. That would remove all the conflicts - and would mean you don't need to duplicate the state handling code...

That is maybe a good idea, like if the wireless remote or wired remote are just buttons, making the wireless remote more dumb. But then there is also the LEDs... maybe this is a bit hard to implement...

Yea fair points both - it's probably quite a bit of re-work. Let's add it to the future todo list :)

rananna commented 3 years ago

@casainho - now we have LEDs for feedback - is it worth looking again at 'virtual throttle' before we get testing for a release?

Are you also ok if I do a tidy up of the code in main.c for the wireless controller. A lot of the code for button handling is spread across various functions that are named for screen type reasons. Needs a tidy up.

@4var1, how do you see virtual throttle working with the remote? It has to be safe and easy to use....

rananna commented 3 years ago

@casainho - now we have LEDs for feedback - is it worth looking again at 'virtual throttle' before we get testing for a release? Are you also ok if I do a tidy up of the code in main.c for the wireless controller. A lot of the code for button handling is spread across various functions that are named for screen type reasons. Needs a tidy up.

@4var1, how do you see virtual throttle working with the remote? It has to be safe and easy to use....

Maybe the hardware shouldn't be virtual. We added a pin for brake control, maybe connect a real throttle to a 52840 pin and connect to the virtual code for throttle control? It would be a lot faster to initiate throttle during a ride and safer to use.

4var1 commented 3 years ago

I'm not sure - isn't something I've used - casainho was quite keen on it though. as a 'get yourself home' feature if you had gear or pedal issues... or you broke your leg!

Maybe we should get everything stable with the base functions first - then look at features....

If we're talking using some more pins - I thought a good use for another two pins would be to reproduce the lights on/off signal - and also add a 'brake light' signal. Both could use similar circuits as the motor on off... But again - probably something to discuss once we get the current functionality sorted.

casainho commented 3 years ago

I think virtual throttle is not a priority for next version, as most users simple do not expect to have that feature.

4var1 commented 3 years ago

yep agreed... let's get what we have tested and released. I'll look at building a remote tonight I think - will even see if I have a button cell so I can test @rananna's power saving efforts!