Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOCS-2084: Add movement sensor and power sensor snippets #3863

Conversation

skyleilani
Copy link
Contributor

@skyleilani skyleilani commented Apr 26, 2024

Is it preferred that the the code snippet for the sensor's Readings() method go in the sensors service's sensors.go file?

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Apr 26, 2024
Copy link
Contributor

Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!

component function
base IsMoving
board GPIOPinByName
camera Properties
encoder Properties
motor IsMoving
sensor Readings
servo Position
arm EndPosition
audio MediaProperties
gantry Lengths
gripper IsMoving
input_controller Controls
movement_sensor LinearAcceleration
power_sensor Power
pose_tracker Poses
motion GetPose
vision ClassificationsFromCamera

@skyleilani skyleilani marked this pull request as draft April 26, 2024 20:44
@npentrel
Copy link
Contributor

This looks good to me aside from the whitespace. For where to place the readings description you'll have to ask the SME reviewer because I'm not sure

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Apr 29, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Apr 29, 2024
@skyleilani skyleilani marked this pull request as ready for review April 29, 2024 14:11
@skyleilani skyleilani requested a review from penguinland May 1, 2024 19:16
Copy link
Member

@penguinland penguinland left a comment

Choose a reason for hiding this comment

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

Where does stuff about Readings go?

It should go in the Sensor interface, rather than being copied between MovementSensor, PowerSensor, etc. That's the DRY (Don't Repeat Yourself) approach, so that if the Sensor interface ever changes, you only need to update that and everything else just points to the updated version.

My feedback is extremely minor, and the rest LGTM!

components/movementsensor/movementsensor.go Outdated Show resolved Hide resolved
components/movementsensor/movementsensor.go Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 7, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 7, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 7, 2024
@viambot viambot removed the safe to test This pull request is marked safe to test from a trusted zone label May 7, 2024
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label May 7, 2024
@skyleilani skyleilani merged commit d2b95d6 into viamrobotics:main May 7, 2024
17 checks passed
@skyleilani skyleilani deleted the DOCS-2084-Add-sensor-movementsensor-powersensor-snippets branch May 7, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants