[WIP] Sync and bulk operations & new actuators

resibots / libdynamixel

C++ interface to the dynamixel actuators

GNU General Public License v2.0

9 stars 15 forks source link

[WIP] Sync and bulk operations & new actuators #61

Open dogoepp opened 5 years ago

dogoepp commented 5 years ago

This is a Work in Progress pull request so that we can discuss about it. I will soon add my comments and suggestions.

PedroDesRobots commented 5 years ago

Ok perfect. I'm waiting for your comments.

dogoepp commented 5 years ago

I do not understand why current-based position control mode is mixed with current control mode in f81be4c. I believe that we should create a new operating mode rather than mix them together when they are not exactly identical. Additionally, I wonder what is the best name for the PWM/voltage mode. "voltage" might be easier to understand, but "pwm" is more coherent with Robotis' documentation. What do you think?

PedroDesRobots commented 5 years ago

Exactly I forgot to recall it correctly. the correct term should be OperatingMode::multi_turn_torque for mode 5.

Actually, PWM is the more common term for controlling a servo. Basically we can call it OperatingMode::PWM but add the information "PWM/voltage" in the method mode2str.

dogoepp commented 5 years ago

Exactly I forgot to recall it correctly. the correct term should be OperatingMode::multi_turn_torque for mode 5.

the documentation mentions that both current and position are controlled in this mode. I don't think that it is multi turn torque then. Am I wrong ?

Actually, PWM is the more common term for controlling a servo. Basically we can call it OperatingMode::PWM but add the information "PWM/voltage" in the method mode2str.

Sounds good. Would you do it ?

Besides, do you know if there finally is a table merging all the control tables of all the servo models, so that we can be sure to not miss implementation details ? It starts to be pretty complex to check the control tables for coherence. If not, I would think on how to build one and have it be updated.

PedroDesRobots commented 5 years ago

current is correlated with torque considering the documentation.
position is associated with multi_turn because it can up to 512 turns which is equivalent to multi_turn (-256[rev] ~ 256[rev]) therefore multi_trun_torque takes all its sense.

I will do it concerning the PWM/voltage modification.

All controls tables are common for a specific servo series. I mean most of the MX servo series share identical control table but sometimes they are some specific address concerning one model. So it's really hard to be consistent, but the idea should be to build one specific control table for each servo series.

PedroDesRobots commented 5 years ago

On some OS, EWOULDBLOCK has the same value as EAGAIN and would hence not compile. Then we decided to comment that.

Why turn this code into a comment? On some OS, EWOULDBLOCK has the same value as EAGAIN and would hence not compile. Then we decided to comment that.

dogoepp commented 5 years ago

I would then suggest either to add OS-specific macros (as JB did in the baudrate files) or to remove this code altogether. I think that the former option is better, if feasible, as it would catch more errors properly.

PedroDesRobots commented 5 years ago

I have tried to add an OS-spefic macro, it compiles on my side (Ubuntu 16.04). The merge with dev-dogoepp is done. Let me know if we need to revise anything else.