V2 - Githubissues

fedeallocati commented 8 years ago

I finally finished the version 2 of the implementation:

Protocol classes that perform the creation and parsing of packets
Individual classes for the different kind of instructions (Ping, Write, Read, Sync Write, etc), that are templated by the Protocol
Servo classes (one for each dynamixel model), using traits to define their Control Tables, and some handy macros to write to the memory (and also to read from the memory and parse the results), both as static and as instance methods:

Mx64 my_mx(1);
ctrl.send(my_mx.set_goal_position(1024));
ctrl.send(Mx64::set_goal_position(1, 1024)); // Exactly same as before

ctrl.send(Mx64::get_present_position(1));
StatusPacket<Mx64::protocol_t> status_packet;
ctrl.recv(status_packet);
std::cout << Mx64::parse_present_position(status_packet) << std::endl;

A base class for all the servos that contains common functionality: Read/write of the fields that all models have (e.g. model number, id, baudrate, goal position, present speed, etc) and functionality that we build on top of the common parts (for example set_goal_position_angle, that computes the exact step for the specified angle)
Some improvement in the error handling: Specific error clases for CRCError (when the dynamixel sends a package and the computed CRC doesn't match the received CRC) and StatusError (the error bit fields in the Status Packet). They both inherit from the base class Error.
A small change in the Usb2Dynamixel controller for the timeout parameter (now it is in the class instead of the recv method

What is missing is the adaption of the examples and tools to the new API, and some higher level functionality like search of the available servos.

I don't expect to merge rigth now, but I need feedback on this. CC: @dogoepp @costashatz @jbmouret

dogoepp commented 8 years ago

Thanks for all that work ! I'll read and comment on the code.

For now, based on your description, what about renaming the base class for exceptions to something specific to dynamixels, like DynamixelError (or put it in the dynamixel namespace) ? I find it misleading when reading code using this library and finding exceptions instances of that type. It doesn't let the programmer know where to look for the definition of the class and what it represents.

I know you kept the existing class, the original name is not your doing. :)

fedeallocati commented 8 years ago

For now, based on your description, what about renaming the base class for exceptions to something specific to dynamixels, like DynamixelError (or put it in the dynamixel namespace) ? I find it misleading when reading code using this library and finding exceptions instances of that type. It doesn't let the programmer know where to look for the definition of the class and what it represents.

Actually they are inside the dynamixel namespace, and also the new errors namespace.

I will also add some comments, so it's clear how to add new models/fix current ones.

fedeallocati commented 8 years ago

I finished the object oriented API. We now have a dynamixel::auto_detect method (in the misc file), with inheritance and model abstraction. With this I think that the V2 is completed and ready to merge. The arm compilation is missing(because I updated the waf version, and their API changed), and also all of the old examples, but we can add that later.

dogoepp commented 8 years ago

In the code, there are many assertions. Are they the proper use cases for assertions ?

cc: @jbmouret @costashatz

costashatz commented 8 years ago

In the code, there are many assertions. Are they the proper use cases for assertions ?

cc: @jbmouret @costashatz

I will have a look this week... I am not convinced yet that all these assertions should be there.

fedeallocati commented 8 years ago

They should'nt be, but it was the quickest way to leave a TODO for throwing exceptions :P

2016-03-01 10:58 GMT+01:00 Konstantinos Chatzilygeroudis < notifications@github.com>:

In the code, there are many assertions. Are they the proper use cases for assertions ?

cc: @jbmouret https://github.com/jbmouret @costashatz https://github.com/costashatz

I will have a look this week... I am not convinced yet that all these assertions should be there.

— Reply to this email directly or view it on GitHub https://github.com/resibots/libdynamixel/pull/6#issuecomment-190641600.

dogoepp commented 8 years ago

It seems that we love typedefs. Then, why don't we use one for the packet's parameters (like here) ? We already use typedefs for instr_t, even though it maps to the same type for protocol 1 and 2.

dogoepp commented 8 years ago

What is the difference between a reg_goal_position_angle and a set_goal_position_angle ?

fedeallocati commented 8 years ago

Reg methods store the value in a temporary memory of the servos, that gets written to the RAM or ROM when you send it an Action instruction. That's what we are using in dynamixel_control_hw to send the joint positions all at the same time: we first register the positions in all the servos, and then broadcast an Action instruction El mar. 2, 2016 8:59, "Dorian Goepp" notifications@github.com escribió:

What is the difference between a reg_goal_position_angle and a set_goal_position_angle ?

— Reply to this email directly or view it on GitHub https://github.com/resibots/libdynamixel/pull/6#issuecomment-191118180.

dogoepp commented 8 years ago

OK thanks

dogoepp commented 8 years ago

As soon as all previous comments are solved, I'm ok for merging.

dogoepp commented 8 years ago

@costashatz Did you have a look at the asserts ? Shall I do this ?

costashatz commented 8 years ago

@dogoepp I will do it this weekend. I would have done it today but I got held up by a bug in hexapod_dart..

On Fri, Mar 4, 2016, 17:38 Dorian Goepp notifications@github.com wrote:

@costashatz https://github.com/costashatz Did you have a look at the asserts ? Shall I do this ?

— Reply to this email directly or view it on GitHub https://github.com/resibots/libdynamixel/pull/6#issuecomment-192346821.

Konstantinos Chatzilygeroudis PhD Candidate Team Larsen INRIA Nancy Grand-Est Room C121 e-mail: konstantinos.chatzilygeroudis@inria.fr web: http://costashatz.github.io/

costashatz commented 8 years ago

@costashatz Did you have a look at the asserts ? Shall I do this ?

@dogoepp @fedeallocati please look at #11 ..

dogoepp commented 8 years ago

Please have a look at #13.

Besides, ready to merge ?

costashatz commented 8 years ago

Please have a look at #13.

Besides, ready to merge ?

Once we merge #13, I will format the code. Then we are ready to merge..

dogoepp commented 8 years ago

When do you think you coud take care of the formating @costashatz ? Do you need help on this ?

dogoepp commented 8 years ago

OK to merge ?

costashatz commented 8 years ago

OK to merge ?

There are conflicts...

dogoepp commented 8 years ago

Because I updated v1 but the related file is deleted in v2 (utility program).

Le 11/03/2016 16:44, Konstantinos Chatzilygeroudis a écrit :

OK to merge ?
There are conflicts...

— Reply to this email directly or view it on GitHub https://github.com/resibots/libdynamixel/pull/6#issuecomment-195421710.

costashatz commented 8 years ago

Because I updated v1 but the related file is deleted in v2 (utility program).

Yes but we need to fix it..

resibots / libdynamixel

V2 #6