Skip to content

Conversation

@jilaypandya
Copy link
Member

rename enable_constant_velocity_mode to run in following files:

  • include/zephyr/drivers/stepper.h
  • include/zephyr/drivers/stepper/stepper_fake.h
  • doc/hardware/peripherals/stepper.rst
  • doc/releases/migration-guide-4.1.rst
  • drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c
  • drivers/stepper/fake_stepper_controller.c
  • drivers/stepper/gpio_stepper_controller.c
  • drivers/stepper/stepper_shell.c
  • tests/drivers/stepper/shell/src/main.c

@jilaypandya jilaypandya self-assigned this Nov 27, 2024
@zephyrbot zephyrbot added Release Notes To be mentioned in the release notes area: Stepper labels Nov 27, 2024
@jilaypandya jilaypandya linked an issue Nov 27, 2024 that may be closed by this pull request
10 tasks
fabiobaltieri
fabiobaltieri previously approved these changes Nov 27, 2024
@jilaypandya jilaypandya added the Enhancement Changes/Updates/Additions to existing features label Nov 27, 2024
dipakgmx
dipakgmx previously approved these changes Nov 27, 2024
faxe1008
faxe1008 previously approved these changes Nov 27, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief run the stepper with a given velocity in a given direction
* @brief Run the stepper with a given velocity in a given direction

Copy link
Contributor

@kartben kartben Nov 28, 2024

Choose a reason for hiding this comment

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

I suggest renaming the value parameter to velocity as its name is now pretty vague otherwise. Please also fix inconsistency below as it says velocity is in steps/s in one place, and steps/us in another.

Also the @details still mentions "constant velocity mode" which might be confusing?

Copy link
Contributor

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Really like the new API name, the velocity_in_usteps_per_second is a bit, wordy, I would go with velocity_ustep_sec and expand its meaning in the @param but its fine either way :)

faxe1008
faxe1008 previously approved these changes Nov 28, 2024
Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

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

I tend to agree with @bjarki-andreasen about the verbosity. Also, did you want to rename the value parameter in the driver implementations too, for clarity? Also wonder about dropping "constant velocity" from L418-L419 in the stepper.h header, maybe just refer to "Run the stepper motor with the given ... / Stop the stepper motor"?
Will eventually approve if you don't feel like changing again, as it's probably borderline nitpicking :) just let me know :)

@jilaypandya
Copy link
Member Author

Will eventually approve if you don't feel like changing again, as it's probably borderline nitpicking :) just let me know :)

I like borderline nitpicking :-) waiting for #82283 to get merged, will then rebase and force push :)

faxe1008
faxe1008 previously approved these changes Dec 3, 2024
kartben
kartben previously approved these changes Dec 3, 2024
fabiobaltieri
fabiobaltieri previously approved these changes Dec 3, 2024
rename enable_constant_velocity_mode to run in following files:
- include/zephyr/drivers/stepper.h
- include/zephyr/drivers/stepper/stepper_fake.h
- doc/hardware/peripherals/stepper.rst
- doc/releases/migration-guide-4.1.rst
- drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c
- drivers/stepper/fake_stepper_controller.c
- drivers/stepper/gpio_stepper_controller.c
- drivers/stepper/stepper_shell.c
- tests/drivers/stepper/shell/src/main.c

Signed-off-by: Jilay Pandya <[email protected]>
@jilaypandya
Copy link
Member Author

sorry guys, the CI should pass now

@kartben kartben merged commit af68d97 into zephyrproject-rtos:main Dec 4, 2024
24 checks passed
@jilaypandya jilaypandya deleted the feature/rename-run branch December 4, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Stepper Enhancement Changes/Updates/Additions to existing features Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stepper API: Rename motion control related functions

7 participants