ergoCubSN001-002: Update amcbdlc and amc versions #684
Conversation
|
Hi @mfussi66 I've seen that the PR is still draft, so I imagine you'll be doing some tests. Ideally, given the small updates, you would be testing the changes on top of the robot-specific fork branches1:
This guarantees that everything remains consistent on the robot setups. Those changes will be then propagated back to upstream (aka here) with periodical PRs. Or, we can do that manually straight away. I'm bringing up this point to get everyone aligned, especially considering that these are the first times we've been testing this approach. Footnotes |
The tests were performed only on ergoCubSN000 (robotology/icub-firmware-models#92) and ergoCubSN001 (robotology/icub-firmware-models#95) On SN000 since the AMC does not perform motor control, I only tested the AMCBLDCs. on SN001 I tested both the boards. SN002 is unavailable at the time of this PR, so i was not able to perform tests, but the boards' features should be the same as SN001, hence why this PR targets that one, too. However, it does not target SN000 since on that robot no check is performed on the AMCBLDC firmware's version, and I did not want to disrupt its operation mode by enforcing a version. The configuration files I used are stored in my fork and the yarprobotinterface boots up only the wrist boards. Let me know if this workflow can be considered equivalent to the contributing guidelines, and if this PR can be merged. I'll mark this as ready for review since the tests were performed, and put it back to draft if deemed insufficient. |
|
The most important point is:
If so, I think we can merge the PR and once AMI decides to align the fork with upstream the boards will need to be updated. This operation is usually performed outright at each Distro release unless there are particular requests.
The tests on the robot should be carried out on the complete setup to avoid underestimating any possible side effects, meaning launching the whole robot with the complete set of conf files. Regarding this
I have a memory of the past when we decided to disable the FW versions because it was creating too many headaches1, cc @GiulioRomualdi. Footnotes
|
Yes, the firmware was restored to the version present in icub-firmware-build contained in the robotology-superbuild folder. So a rebase to devel would require a firmware flash.
Agreed, at the time of testing the robots were not fully functional however, and I would have had to wait for repairs. |
Good 👍🏻
Clear 👍🏻 |
|
Thanks @pattacini for all the caring! Would it be possible to wait for a release of |
|
Sure! |
There was a problem hiding this comment.
About tests:
I am confident to say that since @mfussi66 has tested the binaries of the amc2c and amcbldc on two robots, that is enough to assume that they will work also on the third robot.
So, I am expecting to see six files changed: the -mc_service.xml for left and right wrists for the three ergoCub robots.
About changes:
@mfussi66 I can see only three files changed, so you have probably missed the others.
About merging:
this PR has two other associated PRs that are ready to be merged.
- AMCx mbd: codegen R2024a from icub-fw-models c8834b2 icub-firmware#523
- AMCBLDC v2.1.0, AMC2C v3.1 icub-firmware-build#165
We shall surely merge them together to this one, so that we don't have binaries un-aligned with the xml files.
There was a problem hiding this comment.
@marcoaccame is right.
The right arm of SN002 is missing. @mfussi66 can you update it?
SN000 has version checking disabled as we reported in #684 (comment)
|
Thanks for noticing @marcoaccame and @pattacini , the last commits update the missing file. |
|
Thanks @mfussi66 👍🏻
@S-Dafarra, the corresponding PR on We generally don't produce releases of the build until the new Distro. I think you're ok with rebasing on top of devel. |
Is it a hard dependency? I would like to remain consistent with all the FW of all the boards for a given version of icub-firmware-build. In other words, if we have to update some boards, I would rather update all the boards, otherwise it gets messy. The update usually goes smooth for a release, while for commits it might generate a domino effect with the yarp and icub-main versions. If this cannot be satisfied, I am good in merging this, but I would postpone the rebasing of the robots configuration branches until the next release. Does this sound reasonable? |
|
I think that, in this case, it's safe to update only the boards controlling the wrist of the ergoCub robot. I'm referring here to 2 AMC and 4 AMCBLDC boards handling the left and right wrist. No need to update the middleware or higher levels. Tagging
I would like to merge this PR to remain consistent with the Tip When you aim to update the fork branches is not really critical for us, as we're happy with our tests, I'd say. Important While waiting for the next Distro, if important devs/patches applied to devel will need to land on the physical robots, we will help with the tests/updates. Calling for feedback on this proposal, @S-Dafarra @mfussi66 @marcoaccame @Nicogene |
I would discourage this approach. We have done it a few times in the past and it was never a good situation, ending up in unexpected results. One common assumption is that the robot is aligned to the fw version of icub-firmware-build status in the head/torso. If this is not the case, we might end up in unreproducible situations. |
Did you mean it happened while updating the FW of the wrist?
This would essentially mean that the update will take place only upon fresh tags if I got it right. |
In relation to the latest PRs in icub-firmware (robotology/icub-firmware#523) and icub-firmware-build (robotology/icub-firmware-build#165), this PR updates the boards as per title:
AMC2C : 3.1
AMCBLDC: 2.1