Support stops after pedaling stop with lights on (0.18.1)

[djwlindenaar]

After finishing my install last night, I noticed on the commute to work today that the support sometimes drops out. I need to restart the system (by powerdown using the power button long press and then start) to get it working again.

I have the boost function enabled, but only from standstill. Could this be related? Any debugging actions you'd recommend?

Daniel

[djwlindenaar]

It seems to be related to the lights being turned on. when on, it seems to happen, but not when off. I'll check again, but my ride home is in the dark, so little opportunity to test...

Does this make sense somehow?

Regards Daniel

[djwlindenaar]

Ok. It turns out that it's for sure related to the lights. When driving with lights on, no problem as long as I'm pedaling. As soon as I stop, support is gone. Turn the light off at that moment, support comes back... Turn lights back on, everything is fine until I stop pedaling again...

Daniel

[casainho]

Read the wiki faq about lights.

[djwlindenaar]

Thanks for the tip. Can't find the wiki faq you're referring to, but did find a reference to lights interaction in the forum on endless-sphere.com.

Any clue what causes the system to stop when the lights draw current? Some safety check doing that?

I'd hate to have to integrate additional hardware, I like the simplicity of the current build.

If you can point me in the right direction, I'll try to come up with a patch to solve it

Best regards Daniel

[djwlindenaar]

I've found another issue which is a duplicate of this one, but in the other git repository... Apparently this has been in here for a while.

issue #35

I'll be looking into it based on the hints in that issue and see if I can come up with a patch.

[djwlindenaar]

OK, i've figured out where the issue comes from...

When the lights are turned on, the battery current ADC also measures the current consumed by the lights. The phase current is calculated from this value, which is fine usually, as the error from this additional current is marginal, except at very low duty cycle.

motor.c:475:    ui8_adc_motor_phase_current = ((ui16_adc_battery_current_10b << 6) / ((uint16_t) ui8_duty_cycle));

if the ui16_adc_battery_current_10b is 1 and the ui8_duty_cycle as well, the ui8_adc_motor_phase_current is calculated as 64, which bigger than ADC_MOTOR_PHASE_CURRENT_MAX, therefore the ui8_duty_cycle is immediately decreased in the check (ui8_adc_motor_phase_current > ui8_adc_target_motor_phase_max_current)

So if the current used by the lights is large enough (but not so large to burn out the DC converter) the bike will not start from ui8_duty_cycle == 0. Turning off the lights and then turning them on while driving, it will run OK until previous condition is met again.

Now how to solve this... I'm thinking that at extremely low duty cycles, phase current is going to be low enough anyway, so no protection is needed... Let's say we have a 60V battery, then at 10/256 duty cycle, we never have more than 60 V * 10 / 256 / (0.125 ohm) = 18.75 A of phase current. Right? So a solution could be to only apply the phase current protection if ui8_duty_cycle > 10. Does that make sense?

[ashleysommer]

@djwlindenaar Thanks for looking deeper into this. I have my motor ordered and on its way. I will be doing my install in the next couple of weeks, and I will be commuting in the dark and relying on lights too. Hopefully we can sort this out together.

[ashleysommer]

Looking at the code, it seems like the motor.c code is not able to know whether the lights are on or not. But if it did, you could negatively offset the ui16_adc_battery_current_10b value by a nominal amount (1 amp?) to account for the current a light is likely to be taking, if a lights_on boolean is set.

[djwlindenaar]

@ashleysommer: Yeah, I thought about that, but it's not simple to identify the current drawn by the lights... not sure what happens if we estimate it wrong...

I've done a test drive today and the motor seems to work fine with the ui8_duty_cycle > 10 check. Even with the lights on :smile:

[casainho]

Nice finding!! I wounder if we should look at lights current, to limit it... I think the DC-DC converter for lights already protects in the case of max value.

Maybe the lights current could be measured at startup of the system, like what is done to calibrate the torque sensor. We could enable lights for a short period and measure current.

@djwlindenaar, can you please first figure out what would be the ADC value for your lights? and what would be the max value for max supported current of the hardware??

[djwlindenaar]

@casainho, I can do that, but it'll be a weekend job. I know the LCD shows 0.2 A or 0.4 A, but I'm not sure what the ADC value is. I thought an ADC count is 0.625A, so I'm not sure how 0.2 or 0.4 is measured. Didn't look into that.

[casainho]

Controller send the torque sensor ADC value to LCD3. Just swap to value to be the battery current ADC value and check it on LCD3. Should be quick to do and no risk.

[ashleysommer]

@djwlindenaar do you use the headlamp or tail-lamp or both? Different people might use either or both of them, would be good to get a reading on all light combinations.

[djwlindenaar]

@ashleysommer, I've got both front and rear. For the rear I've put a led strip on the mudguard.

[djwlindenaar]

@casainho, the current is already sent to the kt3:

ui8_tx_buffer[5] = (uint8_t) ((float) motor_get_adc_battery_current_filtered_10b () * 0.826);

So motor_get_adc_battery_current_filtered_10b is flipping between 2 and 3.

Does that help?

Daniel

[casainho]

Good you find that. Maybe you can remove the * 0.826 and see the value.

So, a 2 or 3 units would make all this issue??

[djwlindenaar]

Yeah. ui8_adc_motor_phase_current = ((ui16_adc_battery_current_10b << 6) / ((uint16_t) ui8_duty_cycle)); so of ui8_duty_cycle == 1 and ui16_adc_battery_current_10b == 2 or 3, then the phase current is calculated as 128 or 192... Much more than the max, so no more duty cycle increase...

[casainho]

ui16_adc_battery_current_10b is always positive value. I like the idea of keeping ui16_adc_battery_current_10b = 0 if it is lower than 5, like:

if(ui16_adc_battery_current_10b < 5) ui16_adc_battery_current_10b = 0

Can this work??

[djwlindenaar]

I guess that'll work fine as well. Probably a better idea than what I proposed.

Maybe to avoid a possible limit cycle behaviour due to the step from 0 to 5 we should do:

if(ui16_adc_battery_current_10b < 5)
   ui16_adc_battery_current_10b = 0;
else
  ui16_adc_battery_current_10b -= 5;

[djwlindenaar]

So over the last week I've ridden the bike with the ui8_duty_cycle > 10 check. And it works fine.

I've just implemented the alternative solution from the previous post. It works with the lights on as expected. I'll test-drive it during this week to see how it performs.

[ashleysommer]

@djwlindenaar Thank you for your testing of these solutions. I can't wait to get my motor and do my bike conversation so I can do my part in testing fixes and features too.

[mctubster]

Great job on working out this bug. I'm about to fit the lights to my Cargo bike for winter coming in NZ. Strip lighting on the mudguard for the rear light is a cool idea too as I don't have a rear rack to mount the light to. Suggest LED strip to work with the voltage (6V) and current limitations?

[djwlindenaar]

@mctubster, I got a waterproof USB led strip from AliExpress. Like this one. The 5V vs 6V doesn't matter much, probably. Maybe the lifetime is a bit shorter.

First test drive worked fine, BTW.

[ashleysommer]

@djwlindenaar ~~I just received my motor today, and I plan to flash it with the open-source firmware tonight or tomorrow. Are you able to send me a copy of your build of TSDZ2-v0.18.x.hex with that < 5 modification? I can build it myself but I don't want to mess around with setting up and troubleshooting a working build environment for now. Is your file exactly the same as the TSDZ2-v0.18.2.hex release apart from the lights fix?~~

Don't worry, I built a hex with the mod myself. Do you think having ui16_adc_battery_current_10b always -5 when current_10b is > 5 is ok to do? As far as I can tell, it is only used in calculating the ui8_adc_motor_phase_current and that is only used to check for over-current conditions. So worst-case I guess over-current duty-cycle throttling will occur at ~2 amps higher than before. If that is an issue, we could do something like 0 if < 5, -5 if < 10, -4 if < 20, -3 if < 30, -2 if < 40, -1 if < 50, else no change. That would keep a more accurate current reading at the higher battery currents.

[djwlindenaar]

It's for sure used in more places, however, I cannot check right now. I've implemented the check and -5 in adc.c. I've now driven it for 35 Kms w with lights on and off and no discernable difference in performance compared to before. So you should be ok.

[djwlindenaar]

I've made it a bit fancier as per @ashleysommer 's suggestion. It only costs 19 bytes of extra code memory, so I'd say it's worth it :). The return value goes from 0 to 15 in the space the ADC reading goes from 5 to 15...

@casainho could you give me the access required to create the branch for the pull request?

[ashleysommer]

@djwlindenaar You can fork the Repository into your own Github account, create a branch on that fork, make the change, then you can make a PR to merge the change from your branch to this repository. That is how most people get around the github repository permissions restrictions.

FWIW, this is what I ended up doing in my copy of the code:

    if (ui16_adc_battery_current_10b < 5) {
      ui16_adc_battery_current_10b = 0;
    } else if (ui16_adc_battery_current_10b < 10) {
      ui16_adc_battery_current_10b -= 5;
    } else if (ui16_adc_battery_current_10b < 15) {
      ui16_adc_battery_current_10b -= 4;
    } else if (ui16_adc_battery_current_10b < 20) {
      ui16_adc_battery_current_10b -= 3;
    } else if (ui16_adc_battery_current_10b < 25) {
      ui16_adc_battery_current_10b -= 2;
    }

Its definitely not very elegant, and adds a bunch of branches to the code. I'm sure your solution can be better than that.

[casainho]

Yes, please make a pull request!!!

[casainho]

Pull request merged. Thanks!!

[dzid26]

The phase current is calculated from this value, which is fine usually, as the error from this additional current is marginal, except at very low duty cycle.

motor.c:475:    ui8_adc_motor_phase_current = ((ui16_adc_battery_current_10b << 6) / ((uint16_t) ui8_duty_cycle));

if the ui16_adc_battery_current_10b is 1 and the ui8_duty_cycle as well, the ui8_adc_motor_phase_current is calculated as 64, which bigger than ADC_MOTOR_PHASE_CURRENT_MAX, therefore the ui8_duty_cycle is immediately decreased in the check (ui8_adc_motor_phase_current > ui8_adc_target_motor_phase_max_current)

But but but you guys never questioned why ui8_adc_motor_phase_current is calculated as battery_current / duty_cycle ?... Shouldn't it fundamentally be battery_current x duty_cycle ? I am stumped because "all forks" have the division there.

[djwlindenaar]

I am stumped because "all forks" have the division there.

Well that's because the division actually is correct...

Think about what happens in case of 50% duty and you measure 1A of (averaged) battery current, how much current do you expect is flowing in the motor phase (that's disconnected from the battery power 50% of the time). If that's confusing I suggest a brush-up of your knowledge in half-bridges with PWM and inductive loads :)

[dzid26]

I am stumped because "all forks" have the division there.

Well that's because the division actually is correct...

Think about what happens in case of 50% duty and you measure 1A of (averaged) battery current, how much current do you expect is flowing in the motor phase (that's disconnected from the battery power 50% of the time). If that's confusing I suggest a brush-up of your knowledge in half-bridges with PWM and inductive loads :)

Approximately 2Amps average for inductive loads.

Clever. Thank you for unblocking my brain.

I guess one thing that remains problematic is shift by <<6 and not by <<8 to match 255 range of the duty cycle. (But that's just probably a specific implementation, so don't worry about it.)