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

Add support of the new power-board hardware #15

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

paLeziart
Copy link
Collaborator

@paLeziart paLeziart commented Apr 7, 2022

Description

This PR add a PowerBoard class to handle the new powerboard hardware. It offers a way to retrieve data from the powerboard exposed by the masterboard (current, voltage, energy) through odri control interface.

How I Tested

It was successfully tested with our Solo 12 equipped with a powerboard and I was able to retrieve energy consumption data from my control script.

Do not merge before

The PR to support the powerboard with the masterboard SDK should be merged first, and the masterboard flashed with the updated version of the code.

I fulfilled the following requirements

  • All new code is formatted according to our style guide (for C++ run clang-format, for Python, run flake8 and fix all warnings).
  • All new functions/classes are documented and existing documentation is updated according to changes.
  • No commented code from testing/debugging is kept (unless there is a good reason to keep it).

Copy link
Contributor

@jviereck jviereck left a comment

Choose a reason for hiding this comment

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

This looks very good overall. A few small remarks.

include/odri_control_interface/powerboard.hpp Outdated Show resolved Hide resolved
// 4. Create the calibrator procedure.
// 4. Create the powerboard
std::shared_ptr<Powerboard> powerboard =
std::make_shared<Powerboard>(robot_if);
Copy link
Contributor

Choose a reason for hiding this comment

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

This by default creates a power board. Does this make sense or should there be a flag in the yaml file to indicated if the powerboard is available on a robot or not?

Copy link
Member

Choose a reason for hiding this comment

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

At the moment, the MasterBoard protocol will include all the fields even if a power board is not present. If so the values will read 0.

This is the same with the IMU.

We could in the future have the MasterBoard to auto detect the presence of IMU, PowerBoard and number of uDrivers. According to this, the network packets could be lighten. An expected hardware description via flags in the yaml could then allow to generate an error if some components are missing.

I think this is out of the scope if this PR, and the proposed implementation is coherent with IMU.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for the clearification.

About the master board SDK: Do we need to reflash all our master boards once the software changes to the SDK land that include the power board, even the ones that do not use a power board to get the zero values?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, all master boards will need to be updated together with the sdk.
My advice would be to flash your master board with open-dynamic-robot-initiative/master-board#107 as it's solves old issue we have.

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.

4 participants