search

Frans-Willem / TSDZ2-Clean-EBike

Flexible OpenSource firmware for TongSheng TSDZ2 mid drive ebike motor and pheriperals
GNU General Public License v3.0
3 stars 0 forks source link

[RISK] Cruise and walk-assist can get mixed up #3

Open Frans-Willem opened 5 years ago

Frans-Willem commented 5 years ago

Potential bug: When both cruise and walk assist are enabled, they are activated by the same flag, and then the wheel-speed is used to decide between them.

It's very likely that if you're in walk assist, and add pedal power to go beyond the 8km/h limit, the motor will suddenly switch to cruise mode. Similarly, if you're in cruise and hit the brakes to get it below 8km/h, you may find yourself in walk assist mode. Note that in both of these cases, the LCD would still display the icon for the original mode.

I'll attempt to confirm this bug tomorrow when the weather is better.

Frans-Willem commented 5 years ago

Just had a test-drive, and the behavior luckily isn't as bad as I expected from the controller code. It appears that something (I suspect the LCD) is turning off walk assist (permanently) when you go over 8km/h, and turn off cruise when under 8km/h.

This does not match what appears to be in the controller code (see here). I will have to investigate further.

Frans-Willem commented 5 years ago

It appears the LCD implements the logic to switch off cruise <8km/h, and switch off walk assist >8km/h. This means that although the might be a bug in the controller, the KT-LCD3 firmware masks it. You are, however, at risk when you're running another display (e.g. one of the VLCD5/6 ports).

I'm not upgrading this to bug, but I am going to leave it as an issue as, in my opinion, this is very risky code. Safety logic like this should definitely be in the controller.

Relevant code in LCD: https://github.com/Frans-Willem/TSDZ2-Clean-EBike/blob/master/src/display/KT-LCD3/lcd.c#L1994

leon927 commented 5 years ago

Saw your fork from the Insights, nice job and very clean start!

Actually, there is safety logic in the controller so that users are not able to activate Cruise nor Walk Assist at speeds that are not "supported". Same check performed on two devices. Adds safety when using other displays and during development as well. The defined speed thresholds should be in the common.h to further add another safety layer by using the same parameters.

Also, there is an initialization flag that does not enable Cruise if Walk Assist was first activated and vice versa.

The new code will be even safer in many ways. As an example it will solve all edge cases where users are using the throttle and accidentally use the aforementioned functions.

Frans-Willem commented 5 years ago

The safety checks on the controller side are definitely not in 0.19.0, all that's there is an if condition (ebike.app.c:351) that either does walk assist or cruise depending on the speed at that time. Meaning that if the LCD wouldn't be disabling it, walk assist could turn into cruise control if you go downhill, and cruise control switching to walk assist if you hit the brakes.

They could've been added after 0.19.0, but I'm not in a hurry to pull those commits in seeing as they, in my opinion, don't mix well with the goals I've set for this fork.

leon927 commented 5 years ago

If the display malfunctions and sends the enable signal then yes, it would act the way you describe. Meaning that it would switch on either mode and depending on speed switch between the modes. Sorry if I misunderstood you in any way! Good goals with the safety!