Skip to content

Fix calibration adjustments to move immediately#52

Merged
csader merged 1 commit into
csader:mainfrom
InertiaImpact:fix/CalibrationAdjustmentImmediateMove
Jun 24, 2026
Merged

Fix calibration adjustments to move immediately#52
csader merged 1 commit into
csader:mainfrom
InertiaImpact:fix/CalibrationAdjustmentImmediateMove

Conversation

@InertiaImpact

Copy link
Copy Markdown
Contributor

This pull request refactors how fine-tuning adjustments are sent to hardware modules, ensuring that after a correction is applied, the module immediately moves to the new position for user feedback. It introduces a helper function for building adjustment commands, updates the backend to use this helper, removes redundant client-side requests, and adds tests for the new logic. User-facing descriptions have also been updated for clarity.

Backend improvements:

  • Added a new helper function build_tuning_adjust_commands in tuning.py to generate commands that both save a tuning adjustment and immediately preview it by moving the module.
  • Updated auto_tune_route in server/app.py to use the new helper, ensuring that after an adjustment, the module is moved immediately for feedback. The response now also indicates that the adjustment was previewed.
  • Added unit tests in tests/test_tuning.py to verify that build_tuning_adjust_commands generates the correct commands and properly validates input.

Frontend changes:

  • Removed unnecessary client-side requests in app.js that previously re-sent the same character index after an adjustment, since the backend now handles immediate previewing. [1] [2] [3]

User interface updates:

  • Updated instructional text in index.html to clarify that corrections now move modules immediately, and that negative corrections may require a full forward revolution. [1] [2]

@csader csader left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, well-scoped fix. The extraction into tuning.py with validation and tests is solid, and removing the redundant client-side goto_char requests makes sense now that the backend previews immediately.

One question before I merge: can you confirm the g command (m{id}g{position}) is "goto absolute step position"? The old code was sending goto_char (character index) client-side — the new approach sends a step position directly, which is semantically different. Just want to make sure the firmware handles g as an absolute step goto rather than a character index.

Also noting there's no upper-bound validation on step_position (module_id caps at 254, char_index at 63, but step_position only rejects negatives). Not blocking — firmware should reject out-of-range values — but worth considering for consistency.

Looks good to merge once confirmed.

@InertiaImpact InertiaImpact force-pushed the fix/CalibrationAdjustmentImmediateMove branch from b47c9a6 to 5a347d0 Compare June 24, 2026 18:19
@InertiaImpact

Copy link
Copy Markdown
Contributor Author

As for the g command - it's referenced here as

Moves the reel to an absolute step position (0 to totalStepsPerRev − 1). Sets the current flap index to unknown.

  • after saving the tuned position, we move directly to the adjusted raw step so the compensation is visibly previewed, even if the logical flap index hasn’t changed.

I also added upper-bound validation so the step must be within the module’s calibrated revolution range: 0 <= step_position < calibration_steps.

@csader csader left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, focused change. The g command approach is the right call — re-sending the character index could be ignored since firmware still considers that flap active.

Extraction into tuning.py with validation and tests is solid. Ready to merge.

@csader csader merged commit a12e628 into csader:main Jun 24, 2026
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.

2 participants