Updated RT-TUNE and TUNE-OTHER commands (v1.3)#86
Conversation
lukash
left a comment
lukash
left a comment
There was a problem hiding this comment.
The PR is against main but based on testing, so it's pulling in 42 commits, can you rebase your two commits onto main?
| float onspd = h1; | ||
| float offspd = h2; | ||
| d->float_conf.torquetilt_on_speed = onspd / 2; | ||
| d->float_conf.torquetilt_on_speed = onspd + 3; |
There was a problem hiding this comment.
This changes behavior of the current functionality, so it has the potential to create a mess. You have to be careful to check for the version and if anyone else is actually using this command you're breaking it for them by doing this.
There was a problem hiding this comment.
@surfdado I was referring to this change when talking about the potential breakage on discord. You mentioned something about the len being used to "gate" this, but I don't see it. I'm kinda fine leaving this as a dedicated FC command until there's a better way to set tunes in runtime only, just wanted to make sure there's no misunderstanding or mistake.
There was a problem hiding this comment.
this is correct - in Discord I suggested the "len" gating as a possible solution to address your concern, but hadn't actually implemented it yet. Since it sounds like you are okay with that approach I shall implement it shortly
There was a problem hiding this comment.
Okay, added and updated.
| d->switch_warn_beep_erpm = d->float_conf.is_footbeep_enabled ? 2000 : 100000; | ||
| // bits 2 & 3: | ||
| int pbmode = (flags2 >> 2) & 0x3; | ||
| d->motor_control.parking_brake_mode = pbmode; |
There was a problem hiding this comment.
You don't need this line, the config is propagated to d->motor_control via reconfigure().
There was a problem hiding this comment.
@surfdado I think you removed this line:
d->switch_warn_beep_erpm = d->float_conf.is_footbeep_enabled ? 2000 : 100000;Istead of:
d->motor_control.parking_brake_mode = pbmode;The switch_warn_beep_erpm line is still needed (or you need to move the relevant assignment from configure() to reconfigure(), but feel free to just leave the line, still slated for a refactor, coming in 1.4). But the motor_control line is not.
There was a problem hiding this comment.
ok, another update pushed
|
Also, the changelog trailers should be written from the point of end user, which doesn't know what the RT_TUNE and TUNE_OTHER commands are, could you rephrase the trailers to say something along the lines of "Support <option_list> to be set as a runtime tune (Float Control)"? (feel free to improve it) |
surfdado
force-pushed
the
v130_rttune_cmds
branch
from
aeeb5b0 to
7c58725
Compare
|
Sorry I missed that, should be all fixed now, feature comments are adjusted too. I also added a 3rd commit I had forgotten - adds speed based pushback to the TUNE_TILT command. |
surfdado
force-pushed
the
v130_rttune_cmds
branch
4 times, most recently
from
4e0a424 to
3bec36a
Compare
| d->float_conf.kp2_brake = ((float) h2) / 10; | ||
| beep_alert(d, 1, 1); | ||
| } | ||
| if (len >= 18) { |
There was a problem hiding this comment.
For a safe interface this condition should be >= 19 (to ensure item on index 18 is present) or add a separate if statement for the atr speeds. Otherwise LGTM.
There was a problem hiding this comment.
you're right, changed it
It allows setting mahony kp roll and turntilt start angle. It also allows for higher atr on/off speeds in cfg[18]. Torquetilt speeds can now also be set to higher values (up to 18). Feature: Allow external apps to change features like mahonykp, turntilt start angle, and set faster on/off speeds
…e VarTilt Stays Backwards compatible with older apps Feature: Allow external apps to set DMF, SensorBeep and Parking Brake Mode, as well as negative variable tilt
Feature: Allow external apps to set speed pushback threshold on the fly
surfdado
force-pushed
the
v130_rttune_cmds
branch
from
3bec36a to
1d96cb4
Compare
|
Looking good now, thanks. |
2 participants
This will allow features like negative variable tiltback as well as disable moving faults to be set via COMMAND_TUNE_OTHER. It also has some extensions to COMMAND_RT_TUNE (see individual commits)