Parking brake checks for erpm sign now #34

surfdado · 2025-01-11T15:54:31Z

If rider jumps off the board and the board changes direction after jumping off, it is assumed that the parking brake should be on.

Feature: Parking Brake: Brake immediately if the board would roll the opposite way (e.g. downhill when one jumps off while going uphill)

If rider jumps off the board and the board changes direction after jumping off, it is assumed that the parking brake should be on. Feature: Parking Brake: Brake immediately if the board would roll the opposite way (e.g. downhill when one jumps off while going uphill)

JJJ · 2025-01-14T19:20:34Z

src/motor_control.c

-        mc->parking_brake_active = true;
-    } else if (mc->parking_brake_mode == PARKING_BRAKE_NEVER || state == STATE_RUNNING) {
-        mc->parking_brake_active = false;
+void motor_control_apply(MotorControl *mc, const MotorData *md, RunState state, float time) {


const MotorData *motor is used elsewhere, though I personally think *md is best:

refloat/src/torque_tilt.c

Line 34 in fe9d7b4

void torque_tilt_update(TorqueTilt *tt, const MotorData *motor, const RefloatConfig *config) {

refloat/src/atr.c

Line 53 in fe9d7b4

static void atr_update(ATR *atr, const MotorData *motor, const RefloatConfig *config) {

refloat/src/atr.c

Line 203 in fe9d7b4

ATR *atr, const MotorData *motor, const RefloatConfig *config, float proportional

refloat/src/torque_tilt.h

Line 35 in fe9d7b4

void torque_tilt_update(TorqueTilt *tt, const MotorData *motor, const RefloatConfig *config);

(*m used in other places, so maybe it isn't important, yet?)

refloat/src/motor_data.c

Line 26 in fe9d7b4

void motor_data_reset(MotorData *m) {

refloat/src/motor_data.c

Line 39 in fe9d7b4

void motor_data_configure(MotorData *m, float frequency) {

I tried to be consistent within the line of code I changed, so *md makes sense. But idk why you would even bother commenting on stuff like this. I could care less. Let Lukas name it however he wants.

idk why you would even bother commenting on stuff like this

Because that is how contributing to open-source software works, and how the code gets better over time.

The world reviews requests for progress, and iterates forward together.

I could care less

That's your problem; don't make it mine.

It would be nice to have those consistent, but I'm not gonna spend time going over them to try to unify everything. Sometimes it's a balance of a short name that'll keep the lines from wrapping vs. the name being descriptive enough.

And I'm not gonna bug the PR author with such a small thing if the name is not outright bad.

If you want, @JJJ, you can post a PR unifying all the names of these arguments, I'll merge it 😁

JJJ · 2025-01-14T19:22:55Z

src/motor_control.c

+void motor_control_apply(MotorControl *mc, const MotorData *md, RunState state, float time) {
+    if (mc->parking_brake_mode == PARKING_BRAKE_IDLE) {
+        if (state != STATE_RUNNING &&
+            (md->abs_erpm_smooth < 50 || md->last_erpm_sign != md->erpm_sign)) {


Is 50 an approximation?

Should this be variable/configurable eventually?

this isn't part of my PR

but PLEASE not another parameter for something as mundane as this. 50 seems just fine, if you have arguments why it should be different let's hear them

if you have arguments why it should be different let's hear them

I'm not arguing, or suggesting – I'm asking, while reviewing/observing.

this isn't part of my PR

You're touching this line-of-code and suggesting a change to the surrounding behavior. While we're all here now, look at it, consider it, understand whether 50 is a problem.

It stands out to me as a code-smell that may be contributing to the parking brake feature not behaving as intended, because 50 erpm is unlikely to just be the magic value that makes it work perfectly.

I'm not trying to block your progress or be a jerk, so chill with the defensiveness eh?

I put the 50 there. Seemed to work well to me. This value will not be configurable, it's supposed to represent "0" with a small amount of leeway for erpm noise.

JJJ · 2025-01-14T19:24:05Z

src/motor_data.h

@@ -30,6 +30,7 @@ typedef struct {
    float abs_erpm_smooth;
    float last_erpm;
    int8_t erpm_sign;
+    int8_t last_erpm_sign;  // last erpm sign prior to footpads disengaging


@lukash is probably going to tell you to delete your code comment 😂 because none of the others have them

we need more comments, not less

I agree, but not everyone does

This comment is good, because it's not clear what "last" means in the variable name. It's totally fine this one has a comment and the others don't.

If I was to be extra nitpicky, saying just "... prior to disengaging" would be a bit better.

lukash · 2025-01-16T20:34:02Z

@surfdado thanks for addressing the review, but the actual most important comment was not addressed: #32 (comment)

We need well-structured code more than we need e.g. comments.

Also, is this working well for you? In situations when I had to jump off the board when it wouldn't go up a steep hill, most of the time I'd push the board on the tail just a bit before disengaging, making it shoot downhill. I don't think this implementation would catch it, as it would start the reverse before disengaging.

What I was imagining when we discussed it was last_erpm_sign with a delay, e.g. 1 second. Maybe even 0.5s would do. Not entirely sure about it, I'd have to test it, it'd obviously be a bit harder to implement and this PR is an improvement already, so just something for you to consider if you'd like.

surfdado · 2025-01-20T16:12:25Z

yes, this feature works as it should now for me - it no longer shoots downhill for me on steep climbs when I need to dismount. But of course, if you're already sliding backwards by the time you jump off this feature won't work - and I don't really see a good way around that. A timer based implementation seems hacky to my but if you'd like to do it that way please go ahead.

lukash · 2025-01-21T08:11:36Z

Yes, it would not be pretty and could be considered hacky. I just wanted to bring it up, I won't go testing or doing it now, maybe sometime later, after I see how your implementation does.

Meanwhile, my one remaining point blocking the PR remains without an answer... 😞

JJJ reviewed Jan 14, 2025

View reviewed changes

lukash mentioned this pull request Jan 16, 2025

Parking mode idle improved behavior #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parking brake checks for erpm sign now #34

Parking brake checks for erpm sign now #34

surfdado commented Jan 11, 2025

JJJ Jan 14, 2025 •

edited

Loading

surfdado Jan 14, 2025

JJJ Jan 15, 2025

lukash Jan 16, 2025

JJJ Jan 14, 2025

surfdado Jan 14, 2025

JJJ Jan 15, 2025

lukash Jan 16, 2025

JJJ Jan 14, 2025

surfdado Jan 14, 2025

JJJ Jan 15, 2025

lukash Jan 16, 2025

lukash commented Jan 16, 2025

surfdado commented Jan 20, 2025

lukash commented Jan 21, 2025

Parking brake checks for erpm sign now #34

Are you sure you want to change the base?

Parking brake checks for erpm sign now #34

Conversation

surfdado commented Jan 11, 2025

JJJ Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukash commented Jan 16, 2025

surfdado commented Jan 20, 2025

lukash commented Jan 21, 2025

JJJ Jan 14, 2025 •

edited

Loading