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

Now the joints can run based only to motor encoder - without AMO sensors - and calibrate in hard stop. #438

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

ale-git
Copy link
Contributor

@ale-git ale-git commented Nov 7, 2023

At present, robots like iCub 3 and ErgoCub can use AMO sensors in incremental mode with hard stop calibration.
This new version allows the same robots to run without AMO sensors at all, only relying on motor encoders.

Build PR robotology/icub-firmware-build#112

@valegagge
Copy link
Member

Hi @ale-git,
please add a description of your changes and how they impact on the workflow of the embedded motor control. thank you!

@valegagge
Copy link
Member

valegagge commented Nov 10, 2023

We did some tests at higher and slower velocity: all is ok

20231108_183516.mp4
20231108_183215.mp4

@valegagge valegagge marked this pull request as ready for review November 10, 2023 14:02
@ale-git
Copy link
Contributor Author

ale-git commented Nov 10, 2023

In the motion control library there was still a missing step to be able to control a joint with only motor encoder, hard stop calibrated, with a EMS+2FOC architecture: before starting the hard stop calibration procedure the EMS must wait that the 2FOC itself is calibrated (rotor encoder idex found), otherwise we'll have a hard stop false detection caused by the fact that the 2FOC controlled motor doesn't move until the 2FOC is internally calibrated. In fact, the hard stop detection is base on a timeout euristics: the EMS commands a calibration PWM to the 2FOC, and if the joint doesn't move for a certain time it is assumed that the hard stop is reached. For the reason above, it was already working on MC4+ boards but not on EMS+2FOC boards. Now this PR fixes this gap, providing the right synchronization between EMS and 2FOC during hard stop calibration.

Copy link
Member

@valegagge valegagge left a comment

Choose a reason for hiding this comment

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

Hi @ale-git ,
I reported in the review comments some small changes that I think they are necessary.

After the changes, a the fw should be tested again.

emBODY/eBcode/arch-arm/embobj/plus/mc/JointSet.c Outdated Show resolved Hide resolved
emBODY/eBcode/arch-arm/embobj/plus/mc/JointSet.c Outdated Show resolved Hide resolved
emBODY/eBcode/arch-arm/embobj/plus/mc/Calibrators.c Outdated Show resolved Hide resolved
@valegagge
Copy link
Member

Hi @ale-git ,
regarding the version number of amc fw you could find it here.

@ale-git
Copy link
Contributor Author

ale-git commented Nov 13, 2023

Since the modifications consist in comments and two not used variables I'm going to rebuild the .hex (the amc has still to be done anyway), but there is no need to test it again. Generally speaking, I suggest that our workflow should be corrected: if you want to review the PRs line by line it is well appreciated, but it should be done before the firmware is tested working properly, not after. Testing is a very time consuming activity, and modifications to a already tested and working version makes the previous test a useless waste of time.

@valegagge
Copy link
Member

as discussed f2f with @ale-git, we decided that in the jointSet_calibrate we'll initialize correctly the hard_stop_calib values and in the JointSet_do_wait_calibration_10 we'll use that structure in order to avoid to leave raw values around the code. This small change should improve the maintainability and comprehension of the code.

If in the future we want to make those thresholds parametrized, we just need to set the new parameter in the hard_stop_calib structure in jointSet_calibrate.

We also agreed, that more tests are no longer necessary.

@valegagge
Copy link
Member

Since the modifications consist in comments and two not used variables I'm going to rebuild the .hex (the amc has still to be done anyway), but there is no need to test it again. Generally speaking, I suggest that our workflow should be corrected: if you want to review the PRs line by line it is well appreciated, but it should be done before the firmware is tested working properly, not after. Testing is a very time consuming activity, and modifications to a already tested and working version makes the previous test a useless waste of time.

Yes, I agree with you. we need to plan testing activities better.

@ale-git
Copy link
Contributor Author

ale-git commented Nov 13, 2023

I pushed all the required changes.

@valegagge valegagge self-requested a review November 13, 2023 13:26
Copy link
Member

@valegagge valegagge left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@marcoaccame marcoaccame left a comment

Choose a reason for hiding this comment

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

OK @ale-git, thanks

@marcoaccame marcoaccame merged commit c5780a5 into robotology:devel Nov 13, 2023
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.

3 participants