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 integration for AEA3 encoder #214

Merged
merged 38 commits into from
Nov 24, 2021
Merged

Add integration for AEA3 encoder #214

merged 38 commits into from
Nov 24, 2021

Conversation

sgiraz
Copy link
Contributor

@sgiraz sgiraz commented Nov 9, 2021

This PR contains the new interfaces to interact with the new AEA3 encoder.

Why we should use AEA3 encoders

  • The new AEA3 encoders have a resolution of 14 bits, two more than the older AEA2.
  • Comparing the AEA2-datasheet and AEA3-datasheet, the last one results 88% less noisy.
  • Its physical form factor is very similar to the AEA2, which results in a back-compatible mechanical assembly.

What we need to know before using the new FW (⚠️)

During the integration of the new features, we spotted a critical HW issue that caused many warnings related to a spike detection in the yarprobotinterface.
After a long inspection, this problem has been solved with a temporary HW solution applied on a single mc4plus.
Once the solution will be confirmed, all the other boards will be updated as well.

Note: The new FW has been tested successfully on both AEA2 and AEA3 encoders. So it is back-compatible with the older encoders as well.

The following tests have been performed on an ad-hoc designed setup:

  • test-pwm-noclose: use yarprobotinterface and yarpmotorgui to control the motor in PWM mode at different duty cycle values changing the direction alternately
  • log-joint-pos: a yarp application that moves the joint in position control mode, providing specific positions and logging the feedback from the encoder.

The result of the last test is reported below:

image
image

Other tests have been performed either as a variant of those above or using the yarpmotorgui manually.

Note for FW developers

  • Since there are no bits-state and no zero-padded bits guarantee, using AEA3 in SSI mode we aren't able to check the validity of the reading as we made for AEA2.

Dependencies

This PR is linked with a PR on icub-firmware-shared and with a PR on icub-firmware-build.

Tests

We (@sgiraz and @marcoaccame) performed exhaustive tests of this code both on dedicated setups, on iCubGenova02 and on iCubErzelli02 (courtesy of @Nicogene). After such tests we are confident that the code of this PR and the associated binaries can be merged.

NOTE

Since there are no bits-state and no zero-padded bits guarantee, using AEA3 in SSI mode we aren't able to check the validity of the reading as we made for AEA2.

[@marcoaccame adds:] As a result of that, the function hal_spiencoder_get_value2() always returns hal_res_OK even if there is no encoder attached. This issue is to be addressed by a further PR.

@sgiraz sgiraz requested a review from marcoaccame November 9, 2021 17:02
@sgiraz sgiraz self-assigned this Nov 9, 2021
@pattacini
Copy link
Member

@sgiraz
This is a super big PR, could you please squash it into a single commit and rebase on the latest robotology@devel?
This way, we could prevent possible merging mistakes.

@pattacini
Copy link
Member

We have seen that the fork history is too wild so that we're forced to make use of the Squash and merge button at merge time.

cc @marcoaccame

@marcoaccame
Copy link
Contributor

@sgiraz:

I've just begun to look at this PR. I will come back to you later tomorrow maybe w/ some questions / changes / comments.

You also wrote:

We have to check if the new FW works also on EMS boards.

Let me also verify if we can do this check together, so we can have a solution which is guaranteed to work on all ETH boards.

@marcoaccame
Copy link
Contributor

Hi @sgiraz, now that the dual PR in icub-firmware-shared is merged, I am going to work a bit on this one. Stay tuned.

@marcoaccame
Copy link
Contributor

together w/ @sgiraz we rebased the sgiraz:devel fork vs robotology:devel and fixed some conflicts.
we still need to:

  • update application versions of ems, mc4plus, mc2plus which use the recompiled hal library w/ support to aea3,
  • perform some more checks / tests

and finally we can merge w/ Squash & Merge option

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.

@sgiraz and I need to update application versions and do some extra checks / tests.

sgiraz and others added 21 commits November 23, 2021 16:25
it uses macro TEST_AEA3 to shape used code.
…he way to save the bits retrieved from the sensor in hal_spiencoder.c
… detected. Now it will not rise a spike when the encoder complete a turn.
it uses macro TEST_AEA3 to shape used code.
sgiraz and others added 17 commits November 23, 2021 16:29
…he way to save the bits retrieved from the sensor in hal_spiencoder.c
…ease HEAP to 63K and STACK to 12K

increased application version to be one more than latest devel
@marcoaccame
Copy link
Contributor

marcoaccame commented Nov 24, 2021

@sgiraz and I need to update application versions and do some extra checks / tests.

After the above change request, @sgiraz and I:

  • rebased vs latest devel;
  • adjusted versioning of ems, mc4plus, mc2plus;
  • performed intensive tests on ems and mc4plus and on robots iCubGenova02 and iCubErzelli02;

Now the PR can be merged

@marcoaccame marcoaccame merged commit c9e9b37 into robotology:devel Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants