Skip to content
This repository was archived by the owner on Dec 1, 2020. It is now read-only.

Feature: add incremental encoder #225

Merged
merged 21 commits into from
Mar 5, 2020

Conversation

Olavhaasie
Copy link
Contributor

Closes PM-24

Description

Adds the feature to read out the incremental encoder on the joints. It maps the Motor Position object from the IMotionCube.

@bjornminderman started on this feature and I solved the conflicts it had, since it was a bit outdated.

The original Encoder class is now a super class of AbsoluteEncoder and IncrementalEncoder. Also in order to keep it clean I have added a new encoder directory, where these classes reside. The IMotionCube and Joint class now have two separate functions for reading out the absolute and incremental encoder position. The incremental encoder position is only published on the imc state topic. It shouldn't be too hard to use it for any calculations in the hardware interface.

Changes

  • Create AbsoluteEncoder and IncrementalEncoder
  • Rename a lot of stuff from Encoder to AbsoluteEncoder
  • Add publishing incremental encoder position
  • Add tests

@Olavhaasie Olavhaasie added not-tested-on-march When a PR has not yet been tested on March feature New feature request labels Mar 4, 2020
@Olavhaasie Olavhaasie requested review from bjornminderman and a team as code owners March 4, 2020 13:08
@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

Merging #225 into develop will increase coverage by 0.86%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #225      +/-   ##
===========================================
+ Coverage    52.45%   53.31%   +0.86%     
===========================================
  Files           77       86       +9     
  Lines         2425     2517      +92     
===========================================
+ Hits          1272     1342      +70     
- Misses        1153     1175      +22     
Flag Coverage Δ
#production 34.55% <55.78%> (+0.55%) ⬆️
#test 99.86% <99.17%> (-0.14%) ⬇️
Impacted Files Coverage Δ
...e/include/march_hardware/encoder/AbsoluteEncoder.h 7.14% <0.00%> (ø)
..._hardware/include/march_hardware/encoder/Encoder.h 100.00% <0.00%> (ø)
..._builder/test/incremental_encoder_builder_test.cpp 100.00% <0.00%> (ø)
...rch_hardware/test/mocks/MockIncrementalEncoder.cpp 100.00% <0.00%> (ø)
march_hardware/test/mocks/MockAbsoluteEncoder.cpp 100.00% <0.00%> (ø)
...nclude/march_hardware/encoder/IncrementalEncoder.h 50.00% <0.00%> (ø)
march_hardware/src/encoder/AbsoluteEncoder.cpp 100.00% <0.00%> (ø)
...h_hardware/test/encoder/TestIncrementalEncoder.cpp 100.00% <0.00%> (ø)
march_hardware/src/encoder/IncrementalEncoder.cpp 100.00% <0.00%> (ø)
...are_builder/test/absolute_encoder_builder_test.cpp 100.00% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37e3296...19dd579. Read the comment docs.

Copy link
Contributor

@RutgerVanBeek RutgerVanBeek left a comment

Choose a reason for hiding this comment

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

Nice work @bjornminderman and @Olavhaasie! Main comment is that I think we should for know just read the integer value. The current conversion to radians does not seem appropriate, because we use the total positions, but for an incremental encoder this is bigger than one round.

@Olavhaasie
Copy link
Contributor Author

I will study the behavior of the incremental encoders first

@Roelemans Roelemans removed the not-tested-on-march When a PR has not yet been tested on March label Mar 5, 2020
@Olavhaasie
Copy link
Contributor Author

I have added a transmission variable to the incremental encoder config in b537910. The transmission is already defined in the xacro, however, I was not able to access that with the urdf package, so I added it as a variable in the yaml.

RutgerVanBeek
RutgerVanBeek previously approved these changes Mar 5, 2020
@Olavhaasie Olavhaasie requested a review from Roelemans March 5, 2020 13:02
Copy link
Contributor

@Roelemans Roelemans left a comment

Choose a reason for hiding this comment

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

:)

@RutgerVanBeek RutgerVanBeek self-requested a review March 5, 2020 13:33
@RutgerVanBeek RutgerVanBeek merged commit 28bbd26 into develop Mar 5, 2020
@RutgerVanBeek RutgerVanBeek deleted the feature/PM-24-connect-incremental-encoder branch March 5, 2020 13:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature request
Development

Successfully merging this pull request may close these issues.

4 participants