Skip to content
This repository has been archived by the owner on Jun 19, 2024. It is now read-only.

chore: Fix tests + Examples, clarify docs #37

Merged
merged 7 commits into from
Jan 16, 2024

Conversation

ImpossibleReality
Copy link
Contributor

Describe the changes this PR makes. Why should it be merged?

This PR has a few small changes for consistency and ease of use. It was pretty much the result of me going through the codebase and fixing random things along the way.

This PR:

  • Renames zeroing functions for motor encoders & sensors to provide more context and consistency.
  • Adds Abs trait which allows finding the abs of f64 and f32 types and added to the prelude. (see this issue for more context)
  • Fixes doctests (or at least most of them I think)
  • Fix potential bug with error codes in imu sensor? (take_error was being called more than once, not really sure about this one)
  • Clarifies docs in a few spots

Additional Context

  • These changes require a minor version bump due to marking functions as deprecated
  • These changes update the crate's interface (e.g. functions/modules added or changed).

pros/src/link.rs Outdated Show resolved Hide resolved
Comment on lines 34 to 40
#[deprecate_until(remove = "1.x", note = "Please use the `set_zero` method instead.")]
pub fn zero(&mut self) -> Result<(), PortError> {
self.set_zero()
}

/// Sets the position to zero.
pub fn set_zero(&mut self) -> Result<(), PortError> {
Copy link
Member

@Gavin-Niederman Gavin-Niederman Jan 14, 2024

Choose a reason for hiding this comment

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

Even though you might consider the name zero to be against common Rust conventions, I think that set_zero could easily be confused with set_position. I don't think that this naming change is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whilst I agree that the naming could be confused with setting the position, I do feel that it does clarify the meaning a bit more since the word zero is both a noun and a verb. I think it might be worthwhile in the long run to adopt either Vex's reset terminology or PROS's tare terminology, and have respective reset/tare and reset_to_position/tare_to_position functions on motors/sensors instead.

Copy link
Member

@Tropix126 Tropix126 Jan 14, 2024

Choose a reason for hiding this comment

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

I generally agree with gavin to the extent that "set_zero" is a more confusing name (could be misinterpreted as "set the zero point"). By this naming logic, all device methods should use this terminology (for example, InertialSensor::zero_euler would be changed to InertialSensor::set_zero_euler). At the same time using another term like reset_x would solve both of these problems.

@Tropix126
Copy link
Member

Tropix126 commented Jan 14, 2024

btw, if you want all floating point math functionality on libcore only, num_traits::real::Real recreates those methods through reimplementations leveraging libm (soft floats). I think that's probably out if the scope of this library though, but could be included in the template. I think Abs is also out of scope too though.

@doinkythederp doinkythederp added type:bug Something isn't working type:documentation Improvements or additions to documentation type:enhancement New feature or request semver:minor Adds functionality in a backward compatible manner labels Jan 14, 2024
Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

Don't deprecate the zero function on motors and re-export abs from num instead of writing your own.

@ImpossibleReality
Copy link
Contributor Author

re-export abs from num instead of writing your own.

The abs trait I made compiles to the same assembly instruction as the std abs function on most targets (see this post for more info). I think that what I have is a much more lightweight solution to this, and again, this is meant to be a temporary fix until rust.

Looking back, I partly agree with @Tropix126 in that an Abs trait is a bit out of scope for this project, but I think it could be confusing for end-users not to have a abs function (it was one of the first errors I ran into after trying to make a robot using this library). I'll remove it from this PR for now along with the function renaming so we can merge the PR for now.

@ImpossibleReality ImpossibleReality changed the title chore(minor): Make function naming more consistent, add abs function, clarify docs chore: Make function naming more consistent, add abs function, clarify docs Jan 15, 2024
@ImpossibleReality ImpossibleReality changed the title chore: Make function naming more consistent, add abs function, clarify docs chore: Fix tests + Examples, clarify docs Jan 15, 2024
Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

Add changes in this pr to the unreleased entry in the changelog. After that I think its good to merge.

@Gavin-Niederman
Copy link
Member

Oh, another thing. If you are in the pros-rs discord server, what is your username? I will give you the contributor role once this is merged. If not, here is the invite: https://discord.gg/d4uazRf2Nh. Join if you feel like it.

@ImpossibleReality
Copy link
Contributor Author

PR should be ready to merge. This shouldn't need a minor semver bump anymore since I removed the deprecations. Let me know if anything else needs changing.

Oh, another thing. If you are in the pros-rs discord server, what is your username? I will give you the contributor role once this is merged. If not, here is the invite: https://discord.gg/d4uazRf2Nh. Join if you feel like it.

Just joined today. My username is impossiblereality.

@Gavin-Niederman Gavin-Niederman added semver:patch Backward compatible bug fix and removed semver:minor Adds functionality in a backward compatible manner labels Jan 15, 2024
Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for contributing!

@Gavin-Niederman Gavin-Niederman merged commit cf8b816 into vexide:main Jan 16, 2024
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver:patch Backward compatible bug fix type:bug Something isn't working type:documentation Improvements or additions to documentation type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants