Cut PWM above speed limit (fixes E09)

[dzid26]

I was suspecting the issue was E09 instead of E07 lately. After using your last change 81b813c I was indeed getting the E09 error.

After adding this modification, E07/E09 errors didn't appear.

The change should be done regardless of the errors anyway.

• This is a simple PR that quickly Fixes E07 overcurrent shows up often #99. I have more improvements to this function coming in a next PR.

Rationale:

Speed limit function doesn't limit the PWM ui8_duty_cycle_target. (The PWM is actually set to max value by the assist function during speed limitation.)

I think, as result, motor is not disabled and the only thing stopping the duty cycle from being increased is ui8_adc_battery_current_filtered > ui8_controller_adc_battery_current_target in motor.c. Whenever the current is 0, the pwm will be increased until the current is non-zero. During that time, the motor can accelerate a little bit and trigger ERROR_MOTOR_CHECK, not ERROR_BATTERY_OVERCURRENT.

[emmebrusa]

That's right, duty cycle goes to zero at the end of the deceleration ramp.
I don't like the proposed change, because it cuts the deceleration ramp, it's not good.
Only the code that causes the false error needs to be changed.
The control time of E09 must be greater than the deceleration ramp time + coasting time.
I did a test with deceleration ramp set to 0%.
No errors with E09 control time at 80 (8 seconds).
I would add that I kept E09 (previous E05, motor that goes alone, also called ghost pedaling), but I don't think a real error will occur anymore.

[dzid26]

I see. I didn't think this would be an issue, as at high speed the ramp is typically fast PWM_DUTY_CYCLE_RAMP_DOWN_INVERSE_STEP_MIN anyway.

In fact, I don't think the speed_limit should rely on main pwm ramping to ensure smooth transition. The time constant to reduce speed should be much longer than normal duty cycle ramps and this can be added to the function itself to ramp target current over time - let's say 2 seconds. Currently it is way too abrupt. #71

Let me split the second PR, because it might be relevant to the change you want to make in "moto goes alone" logic.

[emmebrusa]

I saw that you prefer the deceleration ramp to 100%, I thought so too, then I changed my mind.
Now on the MTB with 860C, I have the deceleration ramp at 0%, on the trails I go better, I wouldn't go back anymore.
In the tests consider that too.

[dzid26]

Yes, I definitely I want the torque be reduced together with pedal torque.

This is crucial on technical climbs, when a bit too much torque lifts up the front wheel and you need quick reaction to respond to that. I fell into spiky bushes before because of shmitt trigger filtering and bike was pushing forward a little too long and I went out of balance . :D

But as I said, around the speed limit, the ramp will be usually 100% anyway for you too, due to speed mapping

TSDZ2-Smart-EBike-1/src/ebike_app.c

Lines 637 to 641 in ad782fd

	ui8_duty_cycle_ramp_down_inverse_step = map_ui8((uint8_t)(ui16_wheel_speed_x10>>2),
	(uint8_t)10, // 10*4 = 40 -> 4 kph
	(uint8_t)50, // 50*4 = 200 -> 20 kph
	(uint8_t)ui8_duty_cycle_ramp_down_inverse_step_default,
	(uint8_t)PWM_DUTY_CYCLE_RAMP_DOWN_INVERSE_STEP_MIN);

So I don't think it would be noticeable to set the PWM to zero. Later we can add proper time based current target ramping.

[dzid26]

I was wrong. The speed limit is not abrupt.
I had a bug in my implementation of map_ui16. I thought I "tested" it since I use it in other places. But it was not working correctly with out_max < out_min. For some reason I did not connect the dots, even thought I could see the current target was not decreasing smoothly. Ahhh.

For now I removed map function optimizations. I will add it in a separate PR.

[dzid26]

Ok, so I used that change for some time now. I used it together with the interpolation (map_ui16) fixes that I commited today.

Without map fixes it probably would work too, but unit tests show precision issues against floating point calculation:
Expected target 1860.0mA, got 2080.0mA
Expected target 1240.0mA, got 1600.0mA
Expected target 620.0mA, got 960.0mA, etc...

In any case, again, I don't see a reason not to implement this change:.

• there is no side effect - the current will be almost zero when PWM goes to 0
• it make no sense for the PWM to stay high with motor off
• it fixes the E09 issue
• I rely on it in my new Rev matching feature (spin up the motor together with pedals)

[emmebrusa]

I was thinking about releasing a beta with the E09 fix, but every time I looked at your pull requests I found something changed, and this confused me and made me wait.
Also consider that in the summer I take a break from OSF, and for me it is a problem to try the bike with XH18, I only use the bike with 860C.
But I don't want to wait any longer, and even if I don't have the possibility to try, I wanted to approve in order:

• Cut PWM above speed limit (fixes E09)
• Motor goes alone fixes

Looking at the code changes they seem ok, can you confirm that it is ok?
I would postpone the other pull requests to the fall, when I have time to review and test them.

[dzid26]

• "Cut PWM above speed limit (fixes E09)" is thoroughly tested on the bike and with unit test.
• "Motor goes alone fixes" doesn't cause issues. The change may not be doing much in practise, but eliminates guesswork of what is the value of the timer if motor de | energized repeatedly for whatever reason. I think it's good.