Skip to content

Conversation

@buestad
Copy link
Contributor

@buestad buestad commented Jun 18, 2025

Feature: Added functionality for speed-based pushback and haptic buzz. Configurable in Refloat Cfg

#46

@lukash
Copy link
Owner

lukash commented Jun 18, 2025

Thanks for the PR, skimmed through, looks pretty clean. I'll have to think about the new config options (why not use the same settings as for duty pushback?) and the new pushback SAT (here a specific SAT might be the way to go, but in general I wanna make one SAT be "alert pushback" and then have another enum as the alert reason).

The CI has failed on clang-format, there's a documentation on how to run it in the cotribution guidelines. I'll be busy the next week and a bit, might be slow to response...

@buestad
Copy link
Contributor Author

buestad commented Jun 18, 2025

I Tried running the clang-format (also throug lefthook) but it fails:

$ clang-format -i src/main.c 
.../refloat/.clang-format:7:24: error: invalid boolean
AlignTrailingComments: Never
                       ^~~~~

@buestad
Copy link
Contributor Author

buestad commented Jun 18, 2025

About the config options, It might make sense to use the same angle and angle-speed as duty, but its nice to have a different haptic tone. I don't wanna ignore duty, but speed, not so important :)

@lukash
Copy link
Owner

lukash commented Jun 18, 2025

I Tried running the clang-format (also throug lefthook) but it fails:

$ clang-format -i src/main.c 
.../refloat/.clang-format:7:24: error: invalid boolean
AlignTrailingComments: Never
                       ^~~~~

Looks like you have an old version of clang-format, you need version 18.

@buestad
Copy link
Contributor Author

buestad commented Jun 18, 2025

Looks like you have an old version of clang-format, you need version 18.

Fixed

@lukash
Copy link
Owner

lukash commented Jul 2, 2025

@buestad can you please squash the clang-format fix with the main commit and also drop the merge commits? Instead (if needed) please rebase the branch onto the current main.

@lukash
Copy link
Owner

lukash commented Jul 2, 2025

Also, can you just globally replace board_speed for just speed and analogously for other ways of writing it (SCREAMING_CASE, CamelCase and whatnot) 😁? The word "board" is redundant...

And, in general the goal is to keep the config options minimal necessary and I think we can use the duty settings for mostly everything, the only new option should be the actual speed setting. The options for duty cycle which will now also be used for speed alerting just need to be updated (names and descriptions) to say they now apply to Speed too.

@buestad
Copy link
Contributor Author

buestad commented Jul 2, 2025

I can do the fixes tomorrow 👍 About board_speed. The reason for choosing that was because some other variables in the same area that had speed in the name. Believe it was related to the speed of pushback or something.

@buestad buestad force-pushed the speed_pushback branch 2 times, most recently from c350641 to ac02f02 Compare July 3, 2025 11:50
@buestad buestad force-pushed the speed_pushback branch 2 times, most recently from 2710579 to 7d459b3 Compare July 4, 2025 08:13
@lukash lukash changed the base branch from main to testing July 6, 2025 10:53
@lukash
Copy link
Owner

lukash commented Jul 6, 2025

I changed the target branch for the PR from main to testing, we normally do this for features that should get some testing (and I move them to main manually later on).

buestad and others added 2 commits July 7, 2025 21:24
Feature: Add support for speed-based alerting (Pushback and Haptic)
 >
 Trigger Pushback and Haptic Feedback at a desired speed. The properties
 other than the Speed Threshold are shared with Duty Cycle.
Take the new Speed Pushback into account and clean up some descriptions.
@lukash lukash merged commit 7decad1 into lukash:testing Jul 8, 2025
1 check passed
@acheronfail
Copy link
Contributor

Just wanted to stop by and mention I’ve been running this with a speed limit of 15kmh on a PintV that my son’s been using for 2 weeks and have had no issues whatsoever so far 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants