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

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

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 27 commits into from

Conversation

dogoepp
Copy link
Member

@dogoepp dogoepp commented Mar 26, 2019

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

@PedroDesRobots
Copy link
Contributor

Ok perfect. I'm waiting for your comments.

@dogoepp
Copy link
Member Author

dogoepp commented Apr 2, 2019

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
Copy link
Contributor

PedroDesRobots commented Apr 2, 2019

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
Copy link
Member Author

dogoepp commented Apr 2, 2019

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
Copy link
Contributor

  • 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
Copy link
Contributor

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
Copy link
Member Author

dogoepp commented Jun 18, 2019

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
Copy link
Contributor

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.

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