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

INA260 support #175

Merged
merged 9 commits into from
May 24, 2024
Merged

INA260 support #175

merged 9 commits into from
May 24, 2024

Conversation

Achllle
Copy link
Collaborator

@Achllle Achllle commented Dec 13, 2023

Uses the ina260 library (pip3 install ina260) to get current, voltage, power readouts. Add to default launch.

  • remove existing ina260 code
  • add dependency on ina260 (doesn't seem to be on rosdistro...)
  • write ros 2 node that converts readouts to message and publishes

Future work could include integrating low battery capacity readouts into some warning system.

closes #168

@Achllle Achllle self-assigned this Dec 13, 2023
@apollokit
Copy link
Collaborator

LGTM. I think you only changed one file?

@Achllle
Copy link
Collaborator Author

Achllle commented Jan 24, 2024

Huh, not sure why it didn't get through. I'll make sure the PR is complete after tonight's maintainer meeting. Need to figure out what's going on with my INA in the first place.

@Achllle Achllle marked this pull request as ready for review February 18, 2024 08:08
@Achllle Achllle requested a review from abust005 March 30, 2024 04:03
@@ -0,0 +1,21 @@
import argparse
from ina260.controller import Controller
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused about what file this import statement refers to....I see no ina260.py in this directory on the latest commit on this branch. Am I just missing it somehow?

Does test_ina260.py need to be run with ROS or something? My original intent with the ina260 test files was for them to be completely standalone, with no need to run ROS to test the ina260.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ina260 is a pip package so no library included here!
No ROS required for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, wow, didn't realize that! Alright sounds good. Might be good to add a docstring at the top of the file telling users to pip install it in that case.

@Achllle Achllle merged commit dc51a3a into nasa-jpl:master May 24, 2024
Copy link
Collaborator

@apollokit apollokit left a comment

Choose a reason for hiding this comment

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

Looks good, i'd just add a docstring suggesting to pip install ina206 for the test script

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.

Add software support for INA260
2 participants