Feature/pm 524 Generalize motor controller #284

Roelemans · 2020-08-11T19:22:21Z

Closes PM-524
Requires fix/update-imc-states-msg branch in the march repo

Description

In this PR I generalize the hardware interface for different types of motor controllers. I created an abstract class MotorController that specifies an interface each motor controller needs to implement. I then moved some functionality down from Joint class to the motor controller. Most other changes are the renaming of variables from imc to (motor_)controller.

Currently, the iMotionCube states topic still publishes some imc specific information. I was unsure how I could generalize this, so I decided to postpone this to a different PR. Depending on what exactly the format of the corresponding information for other motor controllers is, I might try to generalize the usage of some of its fields. Alternatively, I could just add more fields for each of the unique states of a motor controller type, or create a different topic for each type. Let me know if you have any ideas.

Changes

Created abstract MotorController class that is implemented by IMotionCube class.
Moved some functionality down to IMotionCube
Renamed lots of variables to (motor_)controller
Check joint for existence of motor controller once during startup and remove all runtime checks

Testing

Imma test on exo tomorrow

…nd fix build

…me checks. Make some small fixes

RutgerVanBeek

I wander what really in what sense the motor controller is more general then the IMC object. It seems that the MotorController object is just a IMC object where everything is virtual. I cannot imagine that other motor controllers are so similar.

march_hardware/include/march_hardware/imotioncube/motor_controller_state.h

march_hardware/include/march_hardware/joint.h

march_hardware/src/ethercat/ethercat_master.cpp

codecov · 2020-08-11T20:23:12Z

Codecov Report

Merging #284 into develop will decrease coverage by 0.03%.
The diff coverage is 42.42%.

@@             Coverage Diff             @@
##           develop     #284      +/-   ##
===========================================
- Coverage    56.66%   56.63%   -0.04%     
===========================================
  Files           88       90       +2     
  Lines         3150     3129      -21     
===========================================
- Hits          1785     1772      -13     
+ Misses        1365     1357       -8

Flag	Coverage Δ
#production	`37.49% <38.31%> (+0.26%)`	⬆️
#test	`98.97% <100.00%> (-0.22%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ude/march_hardware/imotioncube/imotioncube_state.h	`0.00% <0.00%> (ø)`
...arch_hardware/imotioncube/motor_controller_state.h	`0.00% <0.00%> (ø)`
march_hardware/src/ethercat/ethercat_master.cpp	`5.29% <0.00%> (ø)`
...arch_hardware_interface/march_hardware_interface.h	`0.00% <ø> (ø)`
...ardware_interface/src/march_hardware_interface.cpp	`0.00% <0.00%> (ø)`
march_hardware/src/imotioncube/imotioncube.cpp	`11.36% <15.38%> (-0.15%)`	⬇️
march_hardware/src/march_robot.cpp	`20.58% <25.00%> (ø)`
march_hardware/include/march_hardware/joint.h	`57.89% <80.00%> (+17.89%)`	⬆️
march_hardware/src/joint.cpp	`86.20% <82.60%> (+22.73%)`	⬆️
...e/include/march_hardware/imotioncube/imotioncube.h	`50.00% <100.00%> (ø)`
... and 11 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 3876beb...9b5cc5c. Read the comment docs.

Roelemans · 2020-08-11T20:33:20Z

I wander what really in what sense the motor controller is more general then the IMC object. It seems that the MotorController object is just a IMC object where everything is virtual. I cannot imagine that other motor controllers are so similar.

Lemme explain why I think (most of) the methods of the MotorController class are necessary for any motor controller:

All basic necessary functionality is covered by:

getters of things like position, speed and current/torque etc. should be measured by any motor controller
actuation commands for torque/position
some kind of initialization. For the iMC this is split into initialize and goToOperationEnabled, which might not be necessary for all controllers. These can just have an empty method for one of them. I just realized that I did not have a close enough look at initialize and exactly what roll the EthercatMaster, will look into this tomorrow.
Perhaps some kind of shutdown (which can be implemented in the destroyer)

That leaves us with the methods

Every motor controller has some parameters we will want to monitor, hence the getStates method. This might require some additional work to get to work with multiple motor controllers, mainly because one can't subclass ros messages simply (see pr description)
Virtually every motor controller has multiple actuation modes (position/effort/velocity)
getSlaveIndex is relevant for any ethercat motorcontroller and will return -2/-1 for any other to indicate ethercat is unused
checkState is for responding to errors thrown by the motor controller

Does that explain your doubts?

Olavhaasie

Overall, I don't see how this solves any problems. This only creates more. I still have to be convinced of the whole idea of a generalized motor controller, since you only use 1 type, where each motor controller works in its own unique way. The way I see this be used is: a new motor controller gets implemented using the interface, but it becomes clear that it misses some functionality/needs to alter some functionality. The interface gets adapted to work with the new motor controller. However, in the process the IMC gets neglected and never used again, since we now have a new one. Ultimately the IMc gets removed and therefore the whole interface is not useful anymore. I see this as a detour in implementing a new motor controller with a lot of extra maintenance.

march_hardware/CMakeLists.txt

Olavhaasie · 2020-08-12T07:15:24Z

march_hardware/include/march_hardware/imotioncube/motor_controller.h

+  virtual MotorControllerStates getStates() = 0;
+
+  virtual ActuationMode getActuationMode() const = 0;
+  virtual uint16_t getSlaveIndex() const = 0;


What if a motor controller isn't a slave or doesn't use the master-slave structure.

Added a docstring to explain

march_hardware/include/march_hardware/imotioncube/motor_controller.h

Olavhaasie · 2020-08-12T07:17:03Z

march_hardware/include/march_hardware/imotioncube/motor_controller.h

+  virtual double getAngleRadAbsolute() = 0;
+  virtual double getAngleRadIncremental() = 0;
+  virtual double getVelocityRadAbsolute() = 0;
+  virtual double getVelocityRadIncremental() = 0;


What if my motor controller does not have either an absolute or incremental encoder?

I thought about this; the incremental/absolute encoders are a characteristic of Joint rather than of the motor controller, so it would make more sense to move such specifications there. That would require me to subclass Joint into different types, which I preferred to avoid, especially since our current three options for motor controllers will probably be connected to both types of encoders. I think such a refactor of the joint class is better postponed to a moment in the future where an encoder is actually connected to something other than a motor controller.

Hmm on second thought, I could move the calculations using both encoders downstream to the motor controller. Then these would simplify to getPosition() and getVelocity(). Downside would be that these calculations would be that this code would probably be duplicate in each different MotorController class, but that might again be resolvable by inheriting from a MultipleEncodersMotorControllers class that implements these methods common for motor controllers with both encoders.

That was a nice spam of hersenspinsels, let me know if it makes any sense and what you think.

Decided that since the 3 motor controllers currently under consideration all have both an incremental and an absolute encoder, this change is not very useful for now. It won't be difficult to implement later on when such a motor controller is actually put to use.

Olavhaasie · 2020-08-12T07:18:38Z

march_hardware/include/march_hardware/imotioncube/motor_controller.h

+  virtual float getMotorCurrent() = 0;
+  virtual float getMotorControllerVoltage() = 0;
+  virtual float getMotorVoltage() = 0;
+  virtual bool getIncrementalMorePrecise() const = 0;


This also seems like a very specific function that should not be inside the motor controller class. When I read this method call I cannot tell what it would do generally on most controllers.

Added a docstring with explanation. This method is relevant as long as a motor controller has both an incremental and an absolute encoder.

march_hardware/include/march_hardware/joint.h

Olavhaasie · 2020-08-12T07:46:39Z

march_hardware/src/joint.cpp

-    return -1;
-  }
-  return this->imc_->getVelocityIUIncremental();
+  return this->controller_->getTorque();


This will end horribly, if the controller_ is a nullptr, always check you pointers before using them. I now see you do this a lot more, so be warned

I build a check in hardware builder to ensure that every joint has a controller at startup, removing the need to check this every time during runtime.

PS previously, the check at startup was not there, while not everyone of the methods in joint did the nullptr check (e.g. Joint.getIMotionCubeState), so this should actually be an improvement

What if the hardware is not initialized by the hardware builder, what if it contains a bug that results in creating a nullptr? You can't assume that a check somewhere else will protect this class. I know not every method contained the check, but that is not an argument for completely removing it.

Hmm do you think a check in the constructor suffice? That will also immediately cover for all the plebs that appearantly like to make methods without these checks. Also, it will make the code less bloated

The constructor would only check this once. Adding a check here would ensure that it is always a valid pointer.

march_hardware/test/joint_test.cpp

march_hardware_builder/src/hardware_builder.cpp

march_hardware/include/march_hardware/imotioncube/imotioncube.h

…rollerStatus

Olavhaasie

I see you also started renaming things in the MarchRobot class, but not actually changing anything. You are actually making everything more vague and abstract, which is not something we need for software specifically written for one type of hardware. Again, I am still not convinced. To me it seems like you renamed everything from IMotionCube to MotorController plus a few other fixes.

march_hardware/include/march_hardware/motor_controller/imotioncube/imotioncube_states.h

march_hardware/include/march_hardware/motor_controller/motor_controller_states.h

march_hardware/include/march_hardware/motor_controller/imotioncube/imotioncube_states.h

march_hardware/src/ethercat/ethercat_master.cpp

march_hardware/include/march_hardware/motor_controller/imotioncube/imotioncube_states.h

march_hardware/src/imotioncube/imotioncube.cpp

Olavhaasie · 2020-08-17T08:38:27Z

march_hardware/include/march_hardware/march_robot.h

+  void resetMotorControllers();

-  void startEtherCAT(bool reset_imc);
+  void startCommunication(bool reset_motor_controllers);

-  void stopEtherCAT();
+  void stopCommunication();

  int getMaxSlaveIndex();

  bool hasValidSlaves();

-  bool isEthercatOperational();
+  bool isCommunicationOperational();

-  std::exception_ptr getLastEthercatException() const noexcept;
+  std::exception_ptr getLastCommunicationException() const noexcept;

-  void waitForPdo();
+  void waitForUpdate();

-  int getEthercatCycleTime() const;
+  int getCycleTime() const;


It's nice that you changed these names, however, that does not make them compatible with other motor controllers. The reset and start functions are implemented specifically for the IMotionCube, so you still have to change those methods.

I was already hesitant about the removal of ethercat from these namings, because that indeed introduces a lot more vagueness. Hence I reverted them for now.

I do not really understand what you say about reset and start functions; the reset of the MarchRobot directly calls the reset of the MotorController, which should work for any controller. Similarly, the start method calls start for the ethercatMaster, which in turn simply calls initialize for each MotorController. This should work for any Ethercat motor controller. For a none-ethercat motor controller, a new communication master object should be created, with another implementation of the start method.

Olavhaasie · 2020-08-17T08:39:53Z

march_hardware/src/march_robot.cpp

  {
-    ROS_DEBUG("Resetting all IMotionCubes due to either: reset arg: %d or downloading of .sw fie: %d", reset_imc,


Again, nice that you changed these names, however, this makes everything so vague. You name it a motor controller, but in the same sentence use the .sw files, which are specific for the IMotionCube.

A good spot. I changed ".sw file" to "configuration file". Admittedly, that is a bit more vague, but still not to difficult to figure out if you know the system (and this is actually better for noobs that don't know what a .sw file is).

Olavhaasie · 2020-08-17T08:50:51Z

march_hardware/src/joint.cpp

-    return -1;
-  }
-  return this->imc_->getVelocityIUIncremental();
+  return this->controller_->getTorque();


What if the hardware is not initialized by the hardware builder, what if it contains a bug that results in creating a nullptr? You can't assume that a check somewhere else will protect this class. I know not every method contained the check, but that is not an argument for completely removing it.

…some methods

Ishadijcks · 2020-08-24T19:59:12Z

@TimBuckers @martijnvandermarel Here's an interesting issue

ThijsRay

I support the idea that the implementation motor controllers should be more generic, because this would make it easier to implement new motor controllers. However, I am not sure if this is the best interface. Right now, it seems to me that the added layer of abstraction is created with just the iMOTIONCUBE in mind. Other motor controllers might not be able to support all the functionalities that are "required" to implement the MotorController class. It might be beneficial to implement other motor controllers before deciding which methods and functionalities are shared between these implementations. This would avoid the creation of an abstraction that is too specific. Such an abstraction would lose its purpose.

march_hardware/include/march_hardware/ethercat/slave.h

ThijsRay · 2020-08-25T18:15:57Z

march_hardware/include/march_hardware/joint.h

-  std::unique_ptr<IMotionCube> imc_ = nullptr;
-  std::unique_ptr<TemperatureGES> temperature_ges_ = nullptr;
+  std::shared_ptr<MotorController> controller_ = nullptr;
+  std::shared_ptr<TemperatureGES> temperature_ges_ = nullptr;


Why has this been changed to a shared_ptr?

Because I now pass a list of ethercat slaves to ethercat_master (so that march_robot does not need to know which joints are ethercat slaves, and because it makes more sense anyway). So now there are two or three pointers to a a Ges, on in the EthercatMaster, and one in one or two of the joints (previously, the two joints had two separate ges objects for the same ges).

ThijsRay · 2020-08-25T18:22:23Z

march_hardware/include/march_hardware/ethercat/ethercat_master.h

  /**
   * Initializes the ethercat train and starts a thread for the loop.
   * @throws HardwareException If not the configured amount of slaves was found
   *                           or they did not all reach operational state
   */
-  bool start(std::vector<Joint>& joints);
+  bool start();


Could you explain why the joints parameter has been removed and what the return value of this function indicates?

Previously, the joints parameter was used by the ethercat master to access the ethercat slaves to call initSdo on each. The EthercatMaster is now passed a list of ethercat slaves at initiation, making joints parameter unnecessary. The return value is now explained in the docstring

ThijsRay · 2020-08-25T18:22:31Z

march_hardware/include/march_hardware/ethercat/ethercat_master.h

@@ -68,7 +75,7 @@ class EthercatMaster
  /**
   * Configures the found slaves to operational state.
   */
-  bool ethercatSlaveInitiation(std::vector<Joint>& joints);
+  bool ethercatSlaveInitiation();


Could you explain why the joints parameter has been removed and what the return value of this function indicates?

ThijsRay · 2020-08-25T18:25:54Z

march_hardware/include/march_hardware/joint.h

@@ -26,107 +26,57 @@ class Joint
  /**
   * Initializes a Joint with motor controller and without temperature slave.
   */
-  Joint(std::string name, int net_number, bool allow_actuation, std::unique_ptr<IMotionCube> imc);
+  Joint(std::string name, int net_number, bool allow_actuation, std::shared_ptr<MotorController> controller);


Why has this been changed from a unique_ptr to a shared_ptr?

Same reason for the ges, there is now one pointer in EthercatMaster, and one in the MarchRobot

march_hardware_builder/src/hardware_builder.cpp

ThijsRay · 2020-08-25T19:24:23Z

march_hardware_builder/src/hardware_builder.cpp

@@ -196,7 +203,7 @@ HardwareBuilder::createIncrementalEncoder(const YAML::Node& incremental_encoder_
  return std::make_unique<march::IncrementalEncoder>(resolution, transmission);
 }

-std::unique_ptr<march::TemperatureGES> HardwareBuilder::createTemperatureGES(const YAML::Node& temperature_ges_config,
+std::shared_ptr<march::TemperatureGES> HardwareBuilder::createTemperatureGES(const YAML::Node& temperature_ges_config,


Why has this been changed to a shared_ptr?

ThijsRay · 2020-08-25T19:24:51Z

march_hardware_builder/src/hardware_builder.cpp

+  }
+
+  std::shared_ptr<march::TemperatureGES> ges =
+      std::make_unique<march::TemperatureGES>(march::Slave(slave_index, pdo_interface, sdo_interface), byte_offset);


Why are you using make_unique if you want to create a shared_ptr?

Oopsie 😅

march_hardware_builder/src/hardware_builder.cpp

ThijsRay · 2020-08-25T19:25:57Z

march_hardware_builder/src/hardware_builder.cpp

 }

-std::unique_ptr<march::PowerDistributionBoard> HardwareBuilder::createPowerDistributionBoard(
-    const YAML::Node& pdb, march::PdoInterfacePtr pdo_interface, march::SdoInterfacePtr sdo_interface)
+std::shared_ptr<march::PowerDistributionBoard> HardwareBuilder::createPowerDistributionBoard(


Why has this been changed to a shared_ptr?

Roelemans added 10 commits July 16, 2020 13:32

Create MotorController abstract class

6abd775

Merge branch 'develop' into feature/PM-524-add-odrive-option

c8f3af5

Have Joint.cpp use controller instead of imc

f07ebf8

Generalise hardware_builder, hardware_interface and ehtercat master a…

1db4b01

…nd fix build

Fix tests #1

7cf1336

Fix tests #2

9d320e5

Throw a fatal when a joint has no controller defined and remove runti…

fffab68

…me checks. Make some small fixes

Fix error msg

ae33d44

Fix typos

00e9bad

Merge branch 'develop' into feature/PM-524-add-odrive-option

2e1b2f4

Roelemans requested review from bjornminderman, Olavhaasie and a team as code owners August 11, 2020 19:22

Roelemans requested a review from RutgerVanBeek August 11, 2020 19:22

Roelemans self-assigned this Aug 11, 2020

Roelemans added the not-tested-on-march When a PR has not yet been tested on March label Aug 11, 2020

Roelemans requested a review from JorisWeeda August 11, 2020 19:23

RutgerVanBeek suggested changes Aug 11, 2020

View reviewed changes

march_hardware/include/march_hardware/imotioncube/motor_controller_state.h Outdated Show resolved Hide resolved

march_hardware/include/march_hardware/joint.h Show resolved Hide resolved

march_hardware/src/ethercat/ethercat_master.cpp Outdated Show resolved Hide resolved

Roelemans requested a review from RutgerVanBeek August 11, 2020 20:16

Namechange MotorControllerState to MotorControllerStates

ef553b9

Roelemans added 3 commits August 11, 2020 22:44

Fix clang

a60801f

Remove tests untill it works

087227e

Fix build

9d8bc65

Olavhaasie suggested changes Aug 12, 2020

View reviewed changes

Roelemans added 4 commits August 13, 2020 11:30

Generalise imc_state change some naming and add docstrings

81423e5

Moved checkState and getErrorStatus from MotorController to MotorCont…

5e73645

…rollerStatus

Clang

f0c296f

Reduce ethercat dependency in naming

ea89e8e

Roelemans marked this pull request as draft August 14, 2020 10:02

Roelemans and others added 2 commits August 14, 2020 12:02

Merge branch 'develop' into feature/PM-524-add-odrive-option

03c994a

Mock motor controller and fix bugs

df36bbf

Roelemans changed the title ~~Feature/pm 524 add odrive option~~ Feature/pm 524 Generalize motor controller Aug 14, 2020

Olavhaasie suggested changes Aug 17, 2020

View reviewed changes

Roelemans mentioned this pull request Aug 21, 2020

Feature/add odrive two motors #288

Closed

Roelemans added 3 commits August 21, 2020 15:48

Revert generalise ethercat naming

5edecb5

Use Ampere instead of IU for torque, and change input/return type of …

58fbd2f

…some methods

Rename variable in header

855e436

Roelemans marked this pull request as ready for review August 24, 2020 20:05

Roelemans marked this pull request as draft August 24, 2020 20:05

Roelemans added 2 commits August 24, 2020 22:06

Return smart pointer instead of reference and minor fixes

e68a574

Return smart pointer instead of reference and minor fixes #2

0a1c16a

Roelemans requested a review from Olavhaasie August 24, 2020 20:12

Roelemans added 2 commits August 25, 2020 08:54

Merge branch 'develop' into feature/PM-524-add-odrive-option

c49c6b3

Create slave_list object

0fbe358

JorisWeeda requested review from Ishadijcks, martijnvandermarel and TimBuckers August 25, 2020 11:15

Roelemans added 6 commits August 25, 2020 16:15

Pass a slave list to ethercat master

4f66858

Fix tests

b48674d

Fix tests

81f0db4

Remove initialize from Joint and MotorController

41257c8

Move hasValidSlaves() to ethercat master

79a93a3

Remove getSlaveIndex from MotorController

1e55adb

ThijsRay reviewed Aug 25, 2020

View reviewed changes

Roelemans added 2 commits August 26, 2020 19:35

Actually use make_shared for shared_ptrs

ea171c0

Add docstring

9b5cc5c

		{
		ROS_DEBUG("Resetting all IMotionCubes due to either: reset arg: %d or downloading of .sw fie: %d", reset_imc,

Feature/pm 524 Generalize motor controller #284

Are you sure you want to change the base?

Feature/pm 524 Generalize motor controller #284

Conversation

Roelemans commented Aug 11, 2020 • edited Loading

Description

Changes

Testing

RutgerVanBeek left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 11, 2020 • edited Loading

Codecov Report

Roelemans commented Aug 11, 2020 • edited Loading

Olavhaasie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roelemans Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roelemans Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olavhaasie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ishadijcks commented Aug 24, 2020

ThijsRay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roelemans commented Aug 11, 2020 •

edited

Loading

codecov bot commented Aug 11, 2020 •

edited

Loading

Roelemans commented Aug 11, 2020 •

edited

Loading

Roelemans Aug 12, 2020 •

edited

Loading

Roelemans Aug 12, 2020 •

edited

Loading