Wishlist: As I am converting from using Dynamixel SDK...

[KurtE]

Hi @OpusK,

I am trying to get myself going again on the updating of some of Hexapod code base, to now use this library instead of Dynamixel SDK. The main board that I believe Trossen Robotics will use, is the OpenCM9.04... But I would also for my own usage like for it to work well with Teensy boards by PJRC (with my own carrier boards). As I do some of this conversions, I run into different things and or wish things were ... So not sure if this is best place to ask/mention these. Would you prefer that each by separate, so can easily close off as answered?

So far some of the things I have seen include:

a) Wish more (all) of the examples had more complete options for different boards. Example the scan_dynamixel sketch does not have any example of how to configure up to run on OpenCM or OpenCR boards. I did see OpenCM stuff in read_write_ControlTableItem.ino.

b) Wish you would automatically handle somethings like:

#define DXL_SERIAL Serial3 //OpenCM9.04 EXP Board's DXL port Serial. (To use the DXL port on the OpenCM 9.04 board, you must use Serial1 for Serial. And because of the OpenCM 9.04 driver code, you must call Serial1.setDxlMode(true); before dxl.begin();.)

That is maybe on first call to dxl.begin() if compiled for OpenCM and Serial port is Serial1, do the setDXLMode(True)... Or could test again pin 28 in this case...

c) Wishing for some of the member functions including some of the constructors, you allowed pass by pointer as well as by reference. Including ones like:

1. Master::setPort - I may have a set of ports in an arry so may want to do something like: dxl.setPort( with a pointer to port object)... Will see how hard to get arround.
   Interesting you have a getPort which returns a pointer, but not a way to then use it to set it back...
   Not sure if this would work or not:

PortHandler *previous_port = dxl.getPort();
dxl.setPort(my_port);
...
dxl.setPort(*previous_port);

2. SerialPortHandler::SerialPorthandler - WIsh I could pass in HardwareSerial port by pointer...)

3. Dynamixel2Arduino::Dynamixel2Arduino - maybe should allow to pass in port by address and default to nullptr? If I going to setup more than one HardwareSerial objects to pass in a different times, then maybe should be able to initially just be able to declare the main object like:
   Dynamixel2Arduino dxl;

And when I am ready to use it do something like:
dxl.setPort(port_list[index_port_list]);

4. Wish there was easy way to setup to do other forms of SerialPortHandler (subclass?) That is the current version assumes you are using RX, TX and direction pin... But for example with Teensy, I may wish to either use direction pin, in that case I can tell the SerialPort to handle it for me, or I may wish to use half duplex mode (TX Pin), in which case I need to setup in init and set a register bit for which direction I am going ...

Likewise for AVR boards, like Trossen Robotics has the Arbotix boards, which don't use a buffer chip or the like but again toggle states of enable RX or TX pin depending on direction. Obviously I think I could write my own full subclass of the port object to handle this for me, or wonder if ether some of this should be built into default SerialPortHandler and/or setup to easily sub-class...

d) keywords.txt needs a pass through to update with all of the member names like:
setPortProtocolVersionUsingIndex

That is all for now...

[OpusK]

Hi, @KurtE
Thank you for your detailed feedback about this library.

a) Wish more (all) of the examples had more complete options for different boards. Example the scan_dynamixel sketch does not have any example of how to configure up to run on OpenCM or OpenCR boards. I did see OpenCM stuff in read_write_ControlTableItem.ino.
b) Wish you would automatically handle somethings like:

The examples are pin definitions only on the premise of using DynamixelShield.
Previously, there were contents of OpenCM904, but it was removed due to the mixing of Shield and the general board like OpenCM904.
However, as you say, for your convenience, I'll consider adding a README or an example, providing examples of configuration based on the characteristics of the boards.

c) Wishing for some of the member functions including some of the constructors, you allowed pass by pointer as well as by reference. Including ones like:

I'll add one more API that takes a pointer parameter. Thank you for your good feedback.

Dynamixel2Arduino::Dynamixel2Arduino - maybe should allow to pass in port by address and default to nullptr? If I going to setup more than one HardwareSerial objects to pass in a different times, then maybe should be able to initially just be able to declare the main object like:
Dynamixel2Arduino dxl;

The initial design of the APIs was designed only considering the basic circuit of Dynamixel.
Again, I'll add more constructors.

Wish there was easy way to setup to do other forms of SerialPortHandler (subclass?) That is the current version assumes you are using RX, TX and direction pin... But for example with Teensy, I may wish to either use direction pin, in that case I can tell the SerialPort to handle it for me, or I may wish to use half duplex mode (TX Pin), in which case I need to setup in init and set a register bit for which direction I am going ...

In the case of the Master class, just like the SerialPortHandler class, you can inherit from the PortHandler class to create a new class.
However, the Dynamixel2Arduino class declares and uses SerialPortHandler as a member variable for design purposes.
I'll change it to a Pointer so that the user can create and set up a new class that inherits from SerialPortHandler.

d) keywords.txt needs a pass through to update with all of the member names like:
setPortProtocolVersionUsingIndex

I'll add this too.

When I update(PR) the above, I will add you as a reviewer.

[OpusK]

@KurtE

I have created and updated a branch for some of the issues above.

There are still points to consider or think about more. I added an example with respect to the custom SerialPortHandler. I hope this example helps you.

+Update: @KurtE, I hope you'll give a review to #24. Would you please?

[KurtE]

Hi @OpusK,

Will take a look at the new branch:

On examples: My understanding from before is that for the Dynamixel Shield, there is already a different library which is a subclass of this library (DynamixelShield), so obviously those examples should be centered around the shield. However if this library is supposed to be a better light weight library to use for embedded stuff using the OpenCM... then personally think the examples should include this board. (But that is just my $.02 worth)...

A couple of other things I have or will run into.

1. I have an OpenCM9.04 controlling a Hexapod robot, which for me the majority of the time the legs have 3dof (on some earlier non Dynamixel Hexapod I had 4dof). Plus sometimes have a turret with 1 or 2 servos. So I typically need the ability for about 20 servos. Currently I believe the defaults for OpenCM is 16. So need to define some way to expand this. Yes I could edit your library and change your defaults, but this implies that everyone who uses this code will have to do the same.

2. How to use the more extended versions of ping. That is:

XelInfoFromPing_t ping_info;
if (dxl.ping(i, &ping_info, 1)) {

Will fail to compile, likewise:

RecvInfoFromPing_t ping_info_list;
dxl.ping(DXL_BROADCAST_ID, ping_info_list);

Something about C++ when a derived class has a member function with same name as base class, the base class ones are not usable...

I have hacked up a version that compiles, (but I have not tried running yet.
if (((DYNAMIXEL::Master*)&dxl)->ping(i, &ping_info, 1)) {
But hope there is a better way?

[KurtE]

@OpusK,

Forgot to ask, should I do more comments here about changes and/or part of the PR?

Memory(OpenCM) - I understand memory is limited (20K? ) So obviously can not go totally wild on using memory.

So again, the question comes up with what is a valid default size for DXL_MAX_NODE and what does changing that impact and does it make sense to maybe change this separate some of these limitations?
a) Probably most important: The map of ID to Model Numbers in Dynamixel2Arduino.h:
IdAndModelNum_t registered_dxl_[DXL_MAX_NODE];

Currently for openCM board I believe this now takes: 4 bytes per item (as unpacked) so 64 bytes, and if you for example increased this to 32 instead of 16 128 bytes... Could again reduce this by allowing the structure to be packed, example using pragma packed.

Or could go whole hog and allow all 256(or 253) ids and make it a simple array of Model numbers indexed by ID...

And/Or could instead of storing whole ModelNumbers could convert these maybe to a 1 byte value of something like ModelIndex so you don't have to do large case statements for each usage.

b) Ping: again above you could have a default MAX number allowed to be pinged, which again assuming you call the XEL version forms of ping, then you gave me an out, of simply defining my own array to pass in, which solves that... In fact personally I don't see the need to have one with the hard coded max size...

c) SyncWrite (and likewise syncRead probably bulk ones as well). But the main one I currently use is SYncWrite as it was there for Protocol 1...

Personally I don't like the current APIS. Example suppose again suppose I edit DXL_MAX_NODE to just handle the default hexapod with lets say 20 servos.

In order to use the SyncWrite to output the new positions for the servos.
The SyncWrite code is setup that I have:

  typedef struct XelInfoForSyncWriteParam{
    uint8_t id;
    uint8_t data[DXL_MAX_NODE_BUFFER_SIZE];
  } XelInfoForSyncWriteParam_t;

And currently DXL_MAX_NODE_BUFFER_SIZE is 16.
So ParamForSyncWriteInst_t sync_write_param;
Will take up something like: 20*17+9 bytes: 349 bytes.

And in order for you to actually send out the data, you have to then pack the data up and you reserved a buffer in the DxlPacket to handle this (sort of), and you have two of those structures as part of dxl as well.
Note: DXL_BUF_LENGTH would be: 2016+11= 331
#define DXL_BUF_LENGTH (DXL_MAX_NODEDXL_MAX_NODE_BUFFER_SIZE + 11

So will be looking to see if I can do the packing myself, and avoiding double packing.
Sort of along the line of : begin sync write, set sync write data (index, buffer), send it now...

Side note: currently if by the real off chance that someone tries to send 16 bytes of data to all 16 max servos, your packet buffer won't be large enough as you need an extra byte per servo for the id.

Now back to experimenting

[KurtE]

@OpusK - On the ping issue above,

I think the fix for it is to add a using statement in Dynamixel2Arduino.h
That is:

 using DYNAMIXEL::Master::ping;
    bool ping(uint8_t id = DXL_BROADCAST_ID);

I put it just above the new ping member here, but probably could be in any location within the public:
area

Edit: Note: several methods in Dynamixel2Arduino.h comments mention getLastErrorCode() which I don't think exists. But instead in master.h is: getLastLibErrCode

Edit: I have a version of the find all servos OpenCM_Find_Servos which is not that different than yours, although I was experimenting with different versions of ping and the like.
It is not a complete cleaned up version, but it is up in my branch of test github project:
https://github.com/KurtE/Open_CM_CR_Arduino_Sketches/tree/Dynamixel2Arduino

[OpusK]

@KurtE

Thanks for your feedback :)
First of all, I'll explain to you the design concept related to APIs.

1. Dynamixel2Arduino class is for Arduino novice users. Therefore, even if memory waste occurs, users do not have to worry about memory management. However, as you said, I think it's a very inconvenient API for users who can manage memory.

2. The Master class has a more raw API than the Dynamixel2Arduino class. In order to solve some of the memory-related absurdities of Dynamixel2Arduino, I created a form that inherits.

Anyway, an important concept is that Dynamixel2Arduino's API should be easy to use for Arduino novice users.

I and our team had a lot of trouble with this, but now we're just designing the API to this level.
In fact, I still think there's a way to change APIs and memory structures more easily and efficiently.
Therefore, I still leave the major version at 0 (0.x.x). ;)
So, I welcome and appreciate your feedback.

With the feedback you gave me, I will think about it and leave an additional reply.

[OpusK]

@KurtE

The following has been updated so far.

On examples: My understanding from before is that for the Dynamixel Shield, there is already a different library which is a subclass of this library (DynamixelShield), so obviously those examples should be centered around the shield. However if this library is supposed to be a better light weight library to use for embedded stuff using the OpenCM... then personally think the examples should include this board. (But that is just my $.02 worth)...

• Associated commit

On the ping issue above,

• Associated commit

several methods in Dynamixel2Arduino.h comments mention getLastErrorCode() which I don't think exists. But instead in master.h is: getLastLibErrCode

• Associated commit

In addition, the issues related to memory management (Model Index, etc.) need more discussion and consideration.

[KurtE]

@OpusK,
I did sync up and things and looks good.

Memory usage and ease of use stuff, always a good thing to discuss...
Example if I compile your current sync_read_write_raw example app for OpenCM it takes something like: 10024 bytes of RAM. If I edit config.h to increase the number of servos from 16 to 20, this number goes up to 10328 or an increase of 304 bytes (or 76 bytes per servo)

You can save a byte per servo, by packing the IdAndModelNum structure, like:

   typedef struct IdAndModelNum{
      uint16_t model_num;
      uint8_t id;
    } __attribute__((packed)) IdAndModelNum_t ;

Or you can maybe decrease this in half by instead of model_num, you use an uint8_t model index, which could either refer to a actual model number in some table in const memory or could use memory to keep a table of model numbers used...

Or could remove limitation of how many servos you map (but restrict how many unique model numbers to example can only use 15 servo model numbers...
Then could hold an array of 256 4 bit entries (actually could maybe remove last two.
So could simply have, something like

uint8_t registered_dxl_[128] = {0};
uint16_t registerd_dxl_models[15];  // if you really want model numbers...
...
uint16_t Dynamixel2Arduino::getModelNumberFromTable(uint8_t id)
{
    uint8_t model_index = (id & 1)? registered_dxl_[id>>1] >> 4 :  registered_dxl_[id>>1] & 0xf;
    if (model_index != 0) {
        // In our list... Map to model number or index is to table item of usage... 
       // if wanting the actual model, with list above, then maybe
      return registerd_dxl_models[model_index-1];
   }
  // else need to do ping and map to index either by finding it in list above or
  // map model number to some functional usage index, like AX servo, MX servo, ... 
 ... 
}

This would take up More memory but would remove limitations... And we could reduce this, by for example max 8 servo types. Which only need 3 bits. (96 bytes). Code to map to bits would be slightly more complex.

As for SyncWrite like operations, I have some thoughts which I will try to setup in a sketch or the like, which would be a lot easier than trying to type it into this issue...

[KurtE]

@OpusK,

Here is a modified version of the sync_read_write_raw or some such name, that I have two other files included in sketch that so far defines a possible sync write object... It won't run yet as I am not sure how to call something like:
error_code = dxlTxPacketInst(&packet_, DXL_BROADCAST_ID, INST_SYNC_WRITE, _buffer, _cnt_servos * (_node_size+1) + 4);
But you might take a look at the class and changes to sketch to see if this makes sense and would be a easy way to go forward...
Dynamixel2Arduino_sync_read_write-191015a.zip

[KurtE]

Quick update: I hacked up your current version of library (master.h and Master.cpp) and added:
// Experiment allowing some of the TX, RX functions to be called

 bool  Master::txPacketInst(uint8_t id, uint8_t inst_cmd, uint8_t *p_data, uint16_t length) {
  uint32_t pre_time_us = micros();
  last_lib_err_code_ = dxlTxPacketInst(&packet_, id, inst_cmd, p_data, length);
  packet_.tx_time = micros() - pre_time_us;
  return last_lib_err_code_ == DXL_LIB_OK;
 }

And edited the send method of the class I defined in the above zip file:

bool  SyncWrite::send(Dynamixel2Arduino &dxl) {
  // We need to fill in the header information for ths sync write.
  if (!_buffer) return false;

  //uint32_t pre_time_us = micros();
  //lib_err_code_t error_code;

  if (dxl.getPortProtocolVersion() == 1.0) {
    //protocol 1
    _buffer[2] = _starting_addr & 0xff;
    _buffer[3] = _node_size & 0xff;

    return dxl.txPacketInst(DXL_BROADCAST_ID, DYNAMIXEL::INST_SYNC_WRITE, &_buffer[2], _cnt_servos * (_node_size + 1) + 2);
  }
  //protocol 2
  _buffer[0] = _starting_addr & 0xff;
  _buffer[1] = _starting_addr >> 8;
  _buffer[2] = _node_size & 0xff;
  _buffer[3] = _node_size >> 8;
  return dxl.txPacketInst(DXL_BROADCAST_ID, DYNAMIXEL::INST_SYNC_WRITE, _buffer,  _cnt_servos * (_node_size + 1) + 4);
}

It builds, but I have not yet tried it...

[KurtE]

Hi @OpusK,

I had a chance to play around some more with the Sync Read/Write code I mentioned yesterday and made them into classes. These classes allow you to have them allocate memory for them by default or you can pre allocate memory and pass in the buffer to use.

I have update the a version of the sync read/write sketch to do simply set servo goal positions and query back current positions.
Dynamixel2Arduino_sync_read_write-191016a.zip

The above sketch is also in my github project I mentioned earlier: https://github.com/KurtE/Open_CM_CR_Arduino_Sketches/tree/Dynamixel2Arduino

It required a few changes to master.h and master.cpp to allow external libraries/code to be able to call functions to send and receive packets:
master.h I added:

    // Experiment allow external code to call tx, rx functions
   bool  txPacketInst(uint8_t id, uint8_t inst_cmd, uint8_t *p_data, uint16_t length);
   const dxl_packet_t *rxPacket();

And in Master.cpp:

// Experiment allowing some of the TX, RX functions to be called
 bool  Master::txPacketInst(uint8_t id, uint8_t inst_cmd, uint8_t *p_data, uint16_t length) {
  uint32_t pre_time_us = micros();
  last_lib_err_code_ = dxlTxPacketInst(&packet_, id, inst_cmd, p_data, length);
  packet_.tx_time = micros() - pre_time_us;
  return last_lib_err_code_ == DXL_LIB_OK;
 }

const dxl_packet_t *Master::rxPacket() {
  uint32_t pre_time_us = micros();
  last_lib_err_code_ = dxlRxPacket(&packet_);

  // See if we received some valid data...
  if(last_lib_err_code_ != DXL_LIB_OK) 
    return nullptr;
  packet_.rx_time = micros() - pre_time_us;
  return &packet_.rx;
}

Currently the sketch is moving I think 12 servos to 2048+-256.

If you get a chance you might take a look and see what you think.

Also still personally I think it would be great if you hid some of the special case code for OpenCM board to make simple code for Seria1 work without needing user code to do something special.
Example you could do it something like:

void SerialPortHandler::begin(unsigned long baud)
{
#if defined(BOARD_OpenCM904)
  if (_dir_pin == 28) {
      Serial1.setDxlMode(true);
  }
#endif

  baud_ = baud;
  port_.begin(baud_);
  
  if(dir_pin_ != -1){
    pinMode(dir_pin_, OUTPUT);
    digitalWrite(dir_pin_, LOW);
    while(digitalRead(dir_pin_) != LOW);
  }
}

With the code in the #if defined added...

[OpusK]

Hi, @KurtE

Thanks for your feedback and contribution.
I have considered your various proposals and the problems you presented earlier.. (with multi-platform support, etc..)

First of all, Master Class should be modified to change the APIs of Sync and Bulk.

Therefore, if we need to modify the API anyway, I would like to modify the API a lot for the following purposes:

• Support multi-platform (Arduino, bare-metal, OS, etc)
  • I want to write functions such as Master Class in c language for portability.
    • Of course, for Master, I'll wrap it in CPP (class) (with a similar API now).
• Reasonable memory (buffer) allocation
  • As you mentioned, the current memory structure can be stable, but it's inefficient. So, as you suggested, it will support dynamic allocation and custom buffer selection.

Of course, the functions of the Dynamixel2Arduino class will not change. However, if the Master is partially changed and the Sync and Bulk classes are created as you suggested, the related advanced examples will inevitably be modified.

If you have any ideas or opinions as you have been, please tell me.
Thank you again for your contribution.

+Update: In addition, Dynamixel2Arduino currently crashes with the Dynamixel SDK (MACRO and PortHandler, Model Name, etc..) Therefore, this part will be modified, and the function name will be changed. I think it's not a major version of the library yet, so I think I need to change it fast now.

+Update: The expected code layer structure is shown below.

Layer	Contents	Language
4	Dynamixel2Arduino	CPP(same as now)
3	(Wrapping) Master, Slave, PortHandler	CPP(similar to now)
2	Master, Slave, PortHandler(Function Pointer)	C
1	Dynamixel Protocol	C

[OpusK]

@KurtE

d49c0fe

Before changing the layer, I merged some of the suggestions you mentioned in the develop branch.

• OpenCM9.04 setDxlMode in begin of SerialPortHandler.
• raw APIs of Master
• packing of IdAndModelNum structure.

[KurtE]

@OpusK

Thanks for merging all of this stuff into the library. Wonder if we should continue on this issue or startup a new one...

Personally I don't see a need to convert to C, especially with things like PortHandler... As if you wish to support multiple handlers with function pointers, you are now just emulating c++ without the compile type checking. I understand there are some C programmers out there that may wish to do DXL programming. For example: over the last year or so, with the Teensy stuff for the new T4, I sort of reversed some of these versus the T3.x branch. Example HardwareSerial class with T3.x the code was developed like for Serial1, there may have been a c function like: int serial1_available(), which would return the count and the C++ class was simply HardwareSerial1::available() {return serial1_available();}

Now with T4 code we have HardwareSerial::available() doing the work, and a Serial wrappers file (in c++) do something like:

extern "c" {
    int serial1_available() {return Serial1.available();}
}

Again side note: But on T4 code base did some additional things to minimize memory usage with this.
in particular there is HardwareSerial.h (and .cpp), but in addition there is HardwareSerial1.cpp, HardwareSerial2.cpp... HardwareSerial7.cpp

With this for example: HardwareSerial1.cpp is setup to define Serial1 object, plus all of the default buffers it uses. I also put hooks into the main code that before might look something like:

void main () {
    setup();
    for(;;) {
        loop();
        if (Serial.available()) serialEvent();
        if (Serial1.available()) serial1Event();
        if (Serial2.available()) serial2Event();
...
        if (Serial7.available()) serial7Event(); 
    }
}

That instead of explicitly calling the SerialX.available()... functions I instead call on static member of HardwareSerial, where I keep a list of which SerialX.begin() methods have been called. The begins set a function pointer to call as part of their begin. Note: their weak linked function like Serial1Event if called, sets the function pointer back to NULL to not call again...

Why this and why mentioning here? By doing the above, that when you build a sketch that for example does not user Serial2-Serial7, these objects and any necessary buffers are not allocated in that sketch, nor is the overhead then of calling their event code. Although the main reason for indirect calling of the SerialX.available... was to not have any explicit reference to the object which would always bring everything in...

So if you are reworking some of the things like the Master class, you may want to look at what things are tightly coupled into the object, especially if they bring in any additional things like buffers... Example I have never used the Bulk Read and Write stuff.

---

More back to this library and: Support multi-platform (Arduino, bare-metal, OS, etc)

What may turn into an interest challenge is things like which of these platforms support big endian versus little endian? This is another reason why I was migrating over to using methods (functions) to move data in and out of SyncRead/SyncWrite. Example code like this out of sync_read_write_raw
memcpy(sync_write_param.xel[0].data, &velocity, sizeof(velocity));
Will not work properly on the other platforms.

I have not fixed that yet in my class I posted, where instead I do something like:
syncwrite.setItemByIndex(i, &position);

Note: My method also has optional parameters for offset within the data to write as well as size to copy. So if I wish to handle this properly I could add one or more ways to do this. Could do maybe something like explicitly say it is a 4 byte field:
syncwrite.setItemByIndex4(i, &position);
Or could have method where I explicitly pass in the starting register number like:
syncwrite.setItemByIndexRegister(i, &position, offset, DXL_X_GOAL_POSITION);
With this could make passing in the register number optional, as the constructor gives me the starting register number... Of course you may have to do it explicit if using indirect registers....

---

I still wish/want/need support for > 16 servos on OpenCM hopefully by default.

Personally I recommend maybe slightly decoupling of MAX_NODE and MAX_NODE_BUFFER_SIZE from DXL_BUF_LENGTH and/or turn things upside down a little...

That is in config.h where you have something like:

#if defined (ARDUINO_AVR_UNO) || defined (ARDUINO_AVR_YUN) \
  || defined (ARDUINO_AVR_INDUSTRIAL101)
#define DXL_MAX_NODE                   16
#define DXL_MAX_NODE_BUFFER_SIZE       10
#elif defined (ARDUINO_AVR_LEONARDO)
#define DXL_MAX_NODE                   16
#define DXL_MAX_NODE_BUFFER_SIZE       12
#elif defined (OpenCR)
#define DXL_MAX_NODE                  253 // Max number of XEL on DYNAMIXEL protocol
#define DXL_MAX_NODE_BUFFER_SIZE       32
#else
#define DXL_MAX_NODE                   16
#define DXL_MAX_NODE_BUFFER_SIZE       16
#endif

#define DXL_BUF_LENGTH (DXL_MAX_NODE*DXL_MAX_NODE_BUFFER_SIZE + 11) // 11 = Header(3)+Reserved(1)+ID(1)+Length(2)+Instruction(1)+Error(1)+crc(2)

Wish maybe it was more like:

#if defined (ARDUINO_AVR_UNO) || defined (ARDUINO_AVR_YUN) \
  || defined (ARDUINO_AVR_INDUSTRIAL101)
#define DXL_MAX_NODE                   16
#define DXL_BUF_LENGTH                  171 
#elif defined (ARDUINO_AVR_LEONARDO)
#define DXL_MAX_NODE                   16
#define DXL_BUF_LENGTH                  203
#elif defined (OpenCR)
#define DXL_MAX_NODE                  253 // Max number of XEL on DYNAMIXEL protocol
#define DXL_BUF_LENGTH                  8107
#elif defined(Arduino_OpenCM904)
#define DXL_MAX_NODE                   24
#define DXL_BUF_LENGTH                  256    // actually could be 251 to give 10....
#else
#define DXL_MAX_NODE                   16
#define DXL_BUF_LENGTH                  267
#endif

#define DXL_MAX_NODE_BUFFER_SIZE     ((DXL_BUF_LENGTH-11)/ DXL_MAX_NODE)
// 11 = Header(3)+Reserved(1)+ID(1)+Length(2)+Instruction(1)+Error(1)+crc(2)

Probably enough rambling for this round...

[KurtE]

@OpusK

Forgot to mention, it looks like there is no implementation for:
C:\Users\kurte\AppData\Local\Temp\arduino_build_578653\sketch/syncReadWrite.cpp:226: warning: undefined reference to DYNAMIXEL::Master::setLastLibErrCode(DYNAMIXEL::LibErrorCode)'`

Should probably be added to master.cpp at about line 714 like:

void Master::setLastLibErrCode(lib_err_code_t err_code)
{
  last_lib_err_code_ = err_code;
}

[OpusK]

@KurtE

Wonder if we should continue on this issue or startup a new one...

I think it's a good idea to discuss the task of converting to c, memory issues, multiplatform support, etc. in a new thread.

Forgot to mention, it looks like there is no implementation for:
C:\Users\kurte\AppData\Local\Temp\arduino_build_578653\sketch/syncReadWrite.cpp:226: warning: undefined reference to DYNAMIXEL::Master::setLastLibErrCode(DYNAMIXEL::LibErrorCode)'`

Oh, my mistake. I'll fix it.

#if defined (ARDUINO_AVR_UNO) || defined (ARDUINO_AVR_YUN)
|| defined (ARDUINO_AVR_INDUSTRIAL101)
#define DXL_MAX_NODE 16
#define DXL_BUF_LENGTH 171
#elif defined (ARDUINO_AVR_LEONARDO)
#define DXL_MAX_NODE 16
#define DXL_BUF_LENGTH 203

I understand your opinion, but can you tell me why each buffer length is the same as above? I understand 16 and 171 (16 * 10 + 11), but I want to know what standard you did this way. (eg. 16 and 203, That is, a comment is required)

Personally I don't see a need to convert to C

Apart from the Arduino, we want to convert the code to c in case the user wants to use c only (c compiler only) on other platforms.

[KurtE]

@OpusK
New thread it shall be...

As for the changes of defines like I mentioned. Some of it is just my gut reaction about what things are driving what things... That is currently the DXL_BUF_LENGTH is defined to in theory to handle the maximum operation like Sync Write or Sync Read. And it is assumed that all the servos that you may have will be part of that sync write...

Again as I think I sort of mentioned earlier is also not 100% correct.

That is for Sync_Write, your data structures are:

typedef struct XelInfoForSyncWriteParam{
  uint8_t id;
  uint8_t data[DXL_MAX_NODE_BUFFER_SIZE];
} XelInfoForSyncWriteParam_t;

typedef struct ParamForSyncWriteInst{
  uint16_t addr;
  uint16_t length;
  uint8_t id_count;
  XelInfoForSyncWriteParam_t xel[DXL_MAX_NODE];
} ParamForSyncWriteInst_t;

The current defined buffer size is:
#define DXL_BUF_LENGTH (DXL_MAX_NODE*DXL_MAX_NODE_BUFFER_SIZE + 11) // 11 = Header(3)+Reserved(1)+ID(1)+Length(2)+Instruction(1)+Error(1)+crc(2)
So your actual maximum packet would be: Header(3)+Reserved(1)+ID(1)+Length(2)+Instruction(1)+StartingAddress(2)+datalength(2)+DXL_MAX_NODE*(ID(1) + DXL_MAX_NODE_BUFFER_SIZE)+crc(2)
Or for example 16 ids with max 16 data per servo.
14 + 16*17 = 286

Where current actual define is 267

And it is even worse for bulk write as you have ID, addr, length plus data for each node...

So again my comments were just more of a gut reaction. Would more likely change definitions to maybe have MAX IDs separate than number of ids in Sync or bulk operations... As there may be lots of apps who play with servo who never use Sync or Bulk....

But again just rambling

Again I was trying to semi decouple the BUFFER sizes from the off chance you are going to do a

[KurtE]

@OpusK,

Maybe should be on new Issue or Forum or email or ???

If you are interested, for the fun of it, with the sketch I had with potential C++ Sync read and write code, I wrote another version of Sync Read in C. There are two new files that are part of that sketch, and the INO file calls these as well.

Again this is only a test looks like it may be sort of working, obviously if it were real for library would need cleanup and ...

It is up at: https://github.com/KurtE/Open_CM_CR_Arduino_Sketches/tree/Dynamixel2Arduino/Dynamixel2Arduino_sync_read_write

I will probably won't do anything else over the weekend or so...

But the nice thing is, I can do all of this external from library. So only issue left in library is for the default number of IDs that it can map ID to model number...

Hope you have a good weekend

Edit: I updated the test sketch, and I think the C version of sync read it is working a lot better now.

[OpusK]

@KurtE

Maybe should be on new Issue or Forum or email or ???

I think it would be nice to proceed on GitHub issue.

If you are interested, for the fun of it, with the sketch I had with potential C++ Sync read and write code, I wrote another version of Sync Read in C. There are two new files that are part of that sketch, and the INO file calls these as well.

Thanks for sharing!
Currently, I am reworking the functions in protocol.cpp in C style. (Including buffer related)
Master, slave, portHandler, etc. will try to reconsider afterward.
Sooner or later I want to put this on a new branch.
I think it would be nice to review it together afterwards and review your SyncRead C version together.

[KurtE]

@OpusK - I am back from a mini vacation and now recovering ;)

For now I will probably not do much more with the Sync Read/Write, other than keep it in the back of my mind that it is easily doable to not need to use up so much memory on these operations.

So I might now look at how to increase the number of servos supported without killing memory.

Then I may hack up a version of SerialPort - that supports other boards, with or without using Hardware Buffer chips.

My exercise in converting the C++ SyncRead into a C version reminded me several reasons why I don't do straight C anymore. Note: I am not one to use C++ bells ans whistles like the STD libraries, or templates, or dynamic casting, or ...

But some things I do find come in handy:
a) Class and Instance variables and to some extent name space: Old days would create a structure or structures with the member variables and used lots of global variables that could easily conflict... Likewise for conflicts of function names. Then remembering to have to pass in a pointer to the data structure to all the functions associated with the "class"

VTables - In the old days I did do things like, your data structure would contain either a set of member variables or pointer to table of pointers, that you would call. So If you wanted a sub class, you new you had to update the 4th pointer to point to your method... Again all you are doing is emulating C++ here.

Optional/default Arguments on functions: That is these days, I might do something like:
void begin(uint32_t speed=1000000, HardwareSerial *pserial=Serial1, int direction_pin=-1);
And in C++ you would maybe have it setup where you could most of the time get away with simply calling begin () with no parameters...

But in C there is no such thing as default arguments.

Overloading of functions, in C++ I can have two different call signatures by the same name. A simple case is you go well maybe I could do the begin above, by making a couple of versions of it:

void begin();
void begin(uint32_t speed);
void begin(uint32_t speed, HardwareSerial *pserial, int direction_pin);

But that again is not allowed.

So I am keeping my fingers crossed that you won't completely sacrifice the C++ stuff for C.

I am still sort of curious on what platform/setup that people are requiring only C? I know that on many platforms they use GCC compilers, so should also have both compilers... But I understand, you got to do, what you got to do.

Good luck

[KurtE]

@OpusK,

Quick question - I again would like to remove constraint on how many servos that can be handled by different processors. If we disregard the SyncRead/Write likewise Bulk...

One of the main things is a table that gets used to map an ID to a Model number, and my quick look through you are not really interested in the model number. What instead is used is to differentiate some action depending on the type of servo and mostly this is within groups of servos:

And if you go to where these are actually used, they boil down to be used in different switch statements with groups of cases... I sort gathered all of these and for each these I sort of marked groupings of the cases...

--------------------------------------------------------------------------
setBaudrate
--------------------------------------------------------------------------
AX12A: 0
AX12W: 0
AX18A: 0
DX113: 0
DX116: 0
DX117: 0
RX10: 0
RX24F: 0
RX28: 0
RX64: 0
EX106:     0
MX12W: 0
MX28: 0
MX64: 0
MX106: 0
MX28_2: 1
MX64_2: 1
MX106_2: 1
XL430_W250: 1
XC430_W150: 1
XC430_W240: 1
XXL430_W250: 1
XM430_W210: 1
XM430_W350: 1
XH430_V210: 1
XH430_V350: 1
XH430_W210: 1
XH430_W350: 1
XM540_W150: 1
XM540_W270: 1
XH540_W150: 1
XH540_W270: 1
XH540_V150: 1
XH540_V270: 1
PRO_M42_10_S260_R: 2
PRO_M54_40_S250_R: 2
PRO_M54_60_S250_R: 2
PRO_H42_20_S300_R: 2
PRO_H54_100_S500_R: 2
PRO_H54_200_S500_R: 2
PRO_M42_10_S260_RA: 3
PRO_M54_40_S250_RA: 3
PRO_M54_60_S250_RA: 3
PRO_H42_20_S300_RA: 3
PRO_H54_100_S500_RA: 3
PRO_H54_200_S500_RA: 3
PRO_H42P_020_S300_R: 3
PRO_H54P_100_S500_R: 3
PRO_H54P_200_S500_R: 3
PRO_M42P_010_S260_R: 3
PRO_M54P_040_S250_R: 3
PRO_M54P_060_S250_R: 3
---------------------------------------------------------------
bool Dynamixel2Arduino::setLedState(uint8_t id, bool state)
---------------------------------------------------------------
PRO_M42_10_S260_R:
PRO_M54_40_S250_R:
PRO_M54_60_S250_R:
PRO_H42_20_S300_R:
PRO_H54_100_S500_R:
PRO_H54_200_S500_R:
PRO_M42_10_S260_RA:
PRO_M54_40_S250_RA:
PRO_M54_60_S250_RA:
PRO_H42_20_S300_RA:
PRO_H54_100_S500_RA:
PRO_H54_200_S500_RA:
PRO_H42P_020_S300_R:
PRO_H54P_100_S500_R:
PRO_H54P_200_S500_R:
PRO_M42P_010_S260_R:
PRO_M54P_040_S250_R:
PRO_M54P_060_S250_R:
------------------------------------------------------------
bool Dynamixel2Arduino::setOperatingMode(uint8_t id, uint8_t mode)
------------------------------------------------------------
AX12A:  0
AX12W:  0
AX18A:  0
DX113:  0
DX116:  0
DX117:  0
RX10: 0
RX24F:  0
RX28: 0
RX64: 0

EX106: 1
XL320: 2
MX12W: 3
MX28: 3
MX64: 4
MX106: 4
MX28_2: 5
XL430_W250: 5
XC430_W150: 5
XC430_W240: 5
XXL430_W250: 5
MX64_2: 6
MX106_2: 6
XM430_W210: 6
XM430_W350: 6
XH430_V210: 6
XH430_V350: 6
XH430_W210: 6
XH430_W350: 6
XM540_W150: 6
XM540_W270: 6
XH540_W150: 6
XH540_W270: 6
XH540_V150: 6
XH540_V270: 6
PRO_M42_10_S260_R: 7
PRO_M54_40_S250_R: 7
PRO_M54_60_S250_R: 7
PRO_H42_20_S300_R: 7
PRO_H54_100_S500_R: 7
PRO_H54_200_S500_R: 7
PRO_M42_10_S260_RA: 8
PRO_M54_40_S250_RA: 8
PRO_M54_60_S250_RA: 8
PRO_H42_20_S300_RA: 8
PRO_H54_100_S500_RA: 8
PRO_H54_200_S500_RA: 8
PRO_H42P_020_S300_R: 8
PRO_H54P_100_S500_R: 8
PRO_H54P_200_S500_R: 8
PRO_M42P_010_S260_R: 8
PRO_M54P_040_S250_R: 8
PRO_M54P_060_S250_R: 8
-----------------------------------------------------------------------------
ControlTableItemInfo_t DYNAMIXEL::getControlTableItemInfo(uint16_t model_num, uint8_t control_item)
-----------------------------------------------------------------------------
AX12A: 0
AX12W: 0
AX18A: 0
DX113: 0
DX116: 0
DX117: 0
RX10: 0
RX24F: 0
RX28: 0
RX64: 0
EX106: 1
MX12W: 2
MX28: 2
MX64: 3
MX106: 4
MX28_2: 5
MX64_2: 6
MX106_2: 6
XL320: 7
XC430_W150: 8
XC430_W240: 8
XL430_W250: 8
XXL430_W250: 8
XM430_W210: 9
XM430_W350: 9
XH430_V210: 9
XH430_V350: 9
XH430_W210: 9
XH430_W350: 9

XM540_W150: 10
XM540_W270: 10
XH540_W150: 10
XH540_W270: 10
XH540_V150: 10
XH540_V270: 10
PRO_M42_10_S260_R: 11
PRO_M54_40_S250_R: 11
PRO_M54_60_S250_R: 11
PRO_H42_20_S300_R: 11
PRO_H54_100_S500_R: 11
PRO_H54_200_S500_R: 11

PRO_M42_10_S260_RA: 12
PRO_M54_40_S250_RA: 12
PRO_M54_60_S250_RA: 12
PRO_H42_20_S300_RA: 12
PRO_H54_100_S500_RA: 12
PRO_H54_200_S500_RA: 12
PRO_H42P_020_S300_R: 12
PRO_H54P_100_S500_R: 12
PRO_H54P_200_S500_R: 12
PRO_M42P_010_S260_R: 12
PRO_M54P_040_S250_R: 12
PRO_M54P_060_S250_R: 12

So far the biggest distinguishing list is the getControlTableIntemInfo, which appears to have 13 unique tables, now what I have not yet done is to see if some of the other tables conflict. LIke some servo being in one grouping in one operation and a different grouping in a different operation...

What I am wondering is instead of keeping the model number for each servo, we could keep a grouping index number. If it turns out less < 16 could save this info in 4 bits... So to hold information for 254 servos would take 127 bytes. Reserve value 0 maybe as unknown/not in list...

This could allow for much faster mapping. If you want to know which type servo 23 is, you don't have to linear search a list to see if it is in the list and then grab value. Instead you simply compute that this is in byte 12... and grab it...

Also the places that use this like setOperatingMode or the functions that use getControlTableItemInfo would have a much short list of items to use in switch statement and/or could have simple array to load data from...

But before I consider trying this out, does this sound reasonable to you and/or other issues you can think of.

Thanks
Kurt

[OpusK]

Hi, @KurtE

I've been on a short vacation last Friday.
Thank you for your feedback and comments.

First of all, when it comes to converting c++ to c, I thought about it because there was a request from the company.

I agree with many of the things you mentioned.
However, for users who only wanted to use c, they had to do a minimal amount of work.
So, first of all, I decided to implement c only for protocols. (protocol.cpp -> protocol.c)

This work is in progress in the branch below. (I think protocol.c is done)

• code_layer_redesign

I used this to rewrite master and slave.
ping() has changed parameters.
As for sync and bulk, I haven't implemented it yet because I want to talk more with you.
In particular, since the API of Dynamixel2Arduino Class has not changed, the existing examples are not a problem. However, it affects raw examples using the Master function. (as for sync, bulk, ping ...)

And, for packet buffer allocation, I applied your dynamic allocation.
The class is now internally initially allocating a buffer with a size of DXL_BUF_MAX, but I'm also considering how to determine it through class parameters. (Or create a static buffer using a template ...)

Anyway, could you please review the changes?

For your second article, I will read and understand it more and then reply to you.

Thanks again for your contribution!

[OpusK]

@KurtE

And if you go to where these are actually used, they boil down to be used in different switch statements with groups of cases... I sort gathered all of these and for each these I sort of marked groupings of the cases...

I've considered this before, but I've put it on hold for scalability.

What I am wondering is instead of keeping the model number for each servo, we could keep a grouping index number. If it turns out less < 16 could save this info in 4 bits... So to hold information for 254 servos would take 127 bytes. Reserve value 0 maybe as unknown/not in list...

Even if we can reduce it to 4 bits, if we can't reasonably use the additional 4 bits, it looks good to use all 8 bits. BTW, it would be nice to use an 8-bit index for model numbers represented in 16-bit.

Also the places that use this like setOperatingMode or the functions that use getControlTableItemInfo would have a much short list of items to use in switch statement and/or could have simple array to load data from...

It also looks good with arrays instead of switch statements.

[KurtE]

Hi @OpusK,

I meant to reply yesterday, but it was lost... So will try again.

Will take a look at your new branch hopefully in the next few days. I am doing some stuff with Teensy, and was going through and updating some of my current code to use this library, to get ideas of stuff, that I like or don't like.

Have another branch of my fork (https://github.com/KurtE/Dynamixel2Arduino/tree/develop_allow_all_ids) that I was doing some hacking on to allow all IDS... So far it keeps a const PROGMEM table of all currently defined dynamixel types and uses an array of indexes to access these tables. So currently taking 254 bytes of storage, plus program space of Servos:

Am thinking about maybe modifying to either:
a) Maybe modify to only allow up to N unique servo types (example 16, actually 15 as you reserve a value like 0xf to say not used.... Then a 4 bit field would work fine. So could cut the big table in half.
and use a couple simple function/macros to extract/set the servo_type index sort of something like:
servo_type_index = (id & 1)? servo_index_list[id >> 1) >> 4 : servo_index_list[id >> 1) & 0xf;
And then use that value to retrieve the model from your table of up to 15 values... This does nothing for reducing your case statements in the code for models.

b) instead of keeping list of model numbers, we simply define model_groupings, and keep index like above. If we can reduce again down to 15 groups can use same type of calculations and cut in half.

Side note: we could use different number of bits per index other than 4 or 8, it can make the calculations a little more complex although in some cases we can waste a little memory and make it easier still. Like if 15 is too tight, so we wish to go up to 31 groupings.
In this case I might waste a few bits:

(8 bit)Again currently I have: 254 bytes for a byte index (max 255 types)
(4 bit)This could be cut down to 127 bytes for nibble index (max 15 types)
(5 bit) I would probably pack 3 into 16 bits: So could use 159 bytes but I would probably use170
bytes

The 5 bit math is not quite as easy but in logic it would be:

switch (id%3) {
  case 0: return servo_index_list[id/3) & 0x1f;
  case 1: return (servo_index_list[id/3)>>5) & ox1f;
  default: return  (servo_index_list[id/3)>>10;
}

Again we could do it and save the extra few wasted bits, but maybe not worth it...

Got to run... Again will try to take look through your current stuff soon.

Kurt

[KurtE]

@OpusK, Just started to take a look through some of the protocol.h/c stuff.

Wondering: if about if you should or want some form of standardized naming convention for structures and functions?

That is with C++ you can and were using the namespace feature to help make sure that whatever you define should not conflict with other sub-systems...

Looks like you sort of have one with: most of your functions ending with: _dxl_packet and likewise for many of your structures end with: DXLPacket_t

Although all of your error enumeration ones start of with the name DXL...

Wondering (thinking out laud) if all of the structures and maybe functions: maybe should start with DXL?

Also I am still unsure of your audience for C version. With some of your other libraries like Dynamixel Workbench I am/was assuming ones who are likely to use ROS. With ROS they have a standard coding standard. Example with ROS, there is a standard for capitalization and underscores in Class Names, method names, member variables... Wonder if there is also a standard for C versions?

Again sorry for my initial ramblings here.

Luckily so far I am noticing much, as the Protocol stuff is pretty well self contained and only one version of it... (actually 2 but you have them as two separate things)... Now it will be more interesting for PORT, as I personally want the ability to support other platforms, and your stuff you put in earlier sample sketch allows you to sub-class the SerialPortHandler object.

Part of the ability to sub-class the Port handler code may only be needed for maybe three virtual things (or set-able callback functions):
a) Init - Example if my hardware supports half duplex without the need for transmit pin - I may need to have way to call my init function.
b) SetTXMode: instead of hard coded to digitalWrite().. Maybe I need to change a register (or maybe nothing).
c) SetRXMode: again maybe need to do: flush(); followed by changing a register... Or again maybe nothing on Teensy where I can have the underlying system switch for me... (Like OpenCR)...

That is all for now

[KurtE]

Hi @OpusK,

As I mentioned I was playing with the sketch I had the C++ sync read and write classes in, which are running fine on current code_layer_redesign code base.

So I thought I would try implementing the C version, and since I was doing C version, I thought I would start off with your ideas earlier on. But mainly ran into questions on what your thoughts were on it. Again like:

bool syncRead(uint8_t *p_param, uint16_t param_len, uint8_t id_cnt,
              uint8_t *p_recv_buf, uint16_t recv_buf_capacity, uint32_t timeout_ms = 10);

What is expected for each of these arguments:
Somewhere I need to pass in, which register ID to start with, how many bytes to read, and a list of servo IDs. So in this case was my guess correct that p_param points to this data that is already formatted into the right arguments for the SYNC_READ message? (2 byte starting)(2 byte length) followed by the IDS? if so what does id_cnt contain? Is it simply param_len - 4?

Things like that.

Then the next half (or question) was about ok Now if I am going to implement the function syncRead, and I have this data? How to I get it out to the Serial port...
In C++, would simpley do something like:
if (!dxl.txInstPacket(DXL_BROADCAST_ID, DXL_INST_SYNC_READ, p_param, param_len)) return false; // failed on tx
In my Earlier version I simply used a wrapper function:

extern "C" {
  boolean DXLTxPacketInst(uint8_t id, uint8_t instruction, void* buffer, uint16_t length) {
    return dxl.txInstPacket(id, instruction, (uint8_t*)buffer, length);
  }

Are you thinking of doing something like that as well?

Likewise a C wrapper to do the RX?

Some of the other stuff was more a global question on handling big versus little endian.

As one can not assume doing the logical of:

uint32_t current_position;
dxl.read(1, CURRENT_POS, 4, &current_position, 4);

Will work on different platforms, as the byte ordering may be opposite...

Edit: and then how does this impact the users code for the Sync read and write...

again Suppose the user of an XL430... servo wishes to do a SyncRead of (present load, velocity and position), that they might use for example to detect when the leg hits the ground.

So they call the api, with Register 126 length 10, so I have 18 servos, so I pass in a buffer that is at least as long as 18*11 198.

Now what does the user code need to do, to extract the data from this 198 bytes?
maybe something like:
Loop through the data every 11 bytes looking for an ID. Once we find it, suppose we are at byte index 22: (i)
The user code should then do:
load = buff[i+1] +( buff[i+2]<< 8);
vel = buff[i+3] +( buff[i+4]<< 8) + ( buff[i+5]<< 16) + ( buff[i+6]<< 24);
...
Note: probably need casting and like to get right values...
So question, is should there be helper functions to do the like...

Likewise for the Sync Write...

[KurtE]

@OpusK,

Another quick update on playing with C version.
First is the ability for a C file to use any of the data structures and constants in the Dynamixl2Arduino header files. You can not do:

@@ -18,9 +18,13 @@
#define DYNAMIXEL_2_ARDUINO_H_


#ifdef __cplusplus
#include "utility/master.h"
#include "utility/slave.h"
#include "actuator.h"
#else
#include "dxl_c/protocol.h"
#endif

enum OperatingMode{
  OP_POSITION = 0,
@ -48,6 +52,9 @@ enum D2ALibErrorCode
  D2A_LIB_ERROR_UNKNOWN_MODEL_NUMBER
};

// Class only valid in C++
#ifdef __cplusplus

class Dynamixel2Arduino : public DYNAMIXEL::Master
{
  public:
@ -456,5 +463,6 @@ class Dynamixel2Arduino : public DYNAMIXEL::Master



#endif  /* __cplusplus */

#endif /* DYNAMIXEL_2_ARDUINO_H_ */`

This is in diff format. Basically added 2 #ifdef __cplusplus
with else and so far the only thing that is included in C file is the first defines...
and it includes the protocol.h...

Could be done with one #ifdef also could/should also be done, by maybe updating each of the header files includes to only include the C++ stuff when in C++ mode...

Next back to SyncRead:
I put in my two wrapper functions to do writes and reads, to call off in C...
But again wondering about both data going in and data coming out.

Going in: const uint8_t *p_param, uint16_t param_len, uint8_t id_cnt
Again: I am assuming that p_param array already has the starting register as the first two bytes and how many bytes to read in the next two bytes? And then followed by the IDs of the servos to read in.

So what is param_len? Is it the size of the initialized data in p_param? or how many bytes per servo?
Likewise id_cnt again I assume how many Servos to do the read on?

Again assuming param_len is the actual size of data passed in, than id_cnt is redundant, as it should simply be param_len-4.

Received data
Again what are the details of p_recv_buf and recv_buf_capacity?
There are some options on what we may save in here and where.
a) Do we save away the id?
b) Do we save away the error code (err_idx)?
c) and how do we order things.

Example: do we save away the data in the order we receive it. Or does this retrieved data have a specific slot in memory where it should be stored? That is suppose, we query 10 bytes from 18 servos like my previous example: Suppose we request the data from, the ID list:
const uint8_t ServoIDList[] = {19, 3, 5, 2, 4, 6, 7, 9, 11, 8, 10, 12, 13, 15, 17, 14, 16, 18};

And suppose suppose one leg stops working and I don't receive data from servos 2, 4, 6.
And assume the data returned is in the order of the servos in the ID list.
Should the data for ID 7 go into slot 3 or into slot 6?

If Slot 6, then we don't need to store away the servo ID in the results and helper functions like I mentioned might work off of slot # instead of ID... There may be a helper function to convert ID to slot...

Again assuming we wish to keep the err_idx for servos. Do we keep this data spread out in the table, or for example we store them as the first N bytes in the retrieve data. In either case maybe we initialize these slots to some invalid setting, like maybe 0xff to say that servo did not respond... So again we can see which servos did not respond without walking the list...

Again lots of options :D Not sure which ways make more sense to you?

[OpusK]

Hi, @KurtE

Sorry. I was late because of other tasks.

What is expected for each of these arguments:
Somewhere I need to pass in, which register ID to start with, how many bytes to read, and a list of servo IDs. So in this case was my guess correct that p_param points to this data that is already formatted into the right arguments for the SYNC_READ message? (2 byte starting)(2 byte length) followed by the IDS? if so what does id_cnt contain? Is it simply param_len - 4?

I think that the parameters of the API that I declare arbitrarily can cause user confusion.
Rather, the following functions are more convenient to use.

    /* Easy functions for Sync Read */
    bool beginSyncRead(uint16_t addr, uint16_t addr_len);
    bool addSyncReadID(uint8_t id);
    bool sendSyncRead(uint8_t *p_recv_buf, uint16_t recv_buf_capacity);

    /* Easy functions for Sync Write */
    bool beginSyncWrite(uint16_t addr, uint16_t addr_len);
    bool addSyncWriteData(uint8_t id, uint8_t *p_data, uint16_t data_len);
    bool sendSyncWrite();

Are you thinking of doing something like that as well?
Likewise a C wrapper to do the RX?

As I said before, I'm not thinking of making it c except protocol.c.
It would be nice to have a c version for master and slave as well, but for now, I'm thinking of supporting only the CPP version.
However, it seems worth considering the C wrapper you mentioned.

Are you using the Sync feature with the code you created below?

• Link

+Update:

That is sort of how I did it in the past, where I had functions like: setTXMode(), then do the output of the three cunks, and then a setRXMode() to switch back into reading mode. The actual writes don't need to flushed, only when you get back to the setRXMode would you need to flush() and when that completes you would then do your digitalWrite to set chip back to RX mode...

I know what you mean. However, for general and stable operation I have created the same API as now.
Therefore, I made the function Virtual so that the user could modify it to suit their taste.

For example, if you are using DMA, you do not need to use flush and can automatically set the dir pin.

In this case, if you override the write() function, you will be able to optimize the speed.

[KurtE]

@OpusK ,

I can imagine you have lots of stuff going on.

Will take a look at your Easy functions. Not sure if you are expecting that there will only be one set of Sync reads, and Sync write active at any one time...

That is do you see in a loop that the user will always do something like:

    beginSyncRead(DXL_X_PRESENT_POSITION, 4);
    for (uint8_t i=0; i < sizeof(ServoIDList); i++) addSyncReadID(ServoIDList[i]);
    if (sendSyncRead(sync_recv_buffer, sizeof(sync_recv_buffer)) {.... }

Especially if they have multiple ones... Or can the do the begin and the adds in setup, and just call sendSyncRead() in loop?

As for SyncWrite, still an issue of memory. Do you see that the API will malloc or the like buffers as needed? If so, potentially could add another parameter to both beginMethods, like maybe, something like max number of IDS that it will support? Maybe also for those who do not want any heap stuff, maybe add method, like to set buffers? Something like:

    bool beginSyncRead(uint16_t addr, uint16_t addr_len, uint8_t cnt_ids);
    bool addSyncReadID(uint8_t id);
    bool sendSyncRead(uint8_t *p_recv_buf, uint16_t recv_buf_capacity);
    bool setSyncReadBuffer(uint8_t *buffer, uint16_t size); // should be >= 4 + cnt_ids


    /* Easy functions for Sync Write */
    bool beginSyncWrite(uint16_t addr, uint16_t addr_len, cnt_ids);
    bool addSyncWriteData(uint8_t id, uint8_t *p_data, uint16_t data_len);
    bool sendSyncWrite();
    bool setSyncWriteBuffer(uint8_t *buf, uint16_t size);

Will try changing to some of this over the weekend.

Yes that is the link to where several test apps have been. I have not pushed up the most recent of this conversation as it does not compile... But hopefully by Monday...

[OpusK]

@KurtE

Especially if they have multiple ones... Or can the do the begin and the adds in setup, and just call sendSyncRead() in loop?

Yes, it is possible due to the API structure of the current protocol.
However, if you create several sync packets and want to use only the buffers (packets), this is difficult in the current way. This method seems to work if I combine it well with the method you suggested below.

bool setSyncReadBuffer(uint8_t *buffer, uint16_t size); // should be >= 4 + cnt_ids

I was considering this function as well.
I thought I could create and use different Sync packets in different buffers.

Of course, I'm wondering if it's better to add this to the Master class or use a separate class as you created.

[KurtE]

@OpusK,

Yesterday I did hack in a version of syncRead along the line of your APIs as part of the one test sketch and update the main sketch to call it.

It builds, but I have not tested it yet... But I did push it up the github project in case you wanted a look.

So far I have not put in any of the set Buffers, but did add some query functions to extract the data retrieved from the buffer. Currently I have the buffer encoded with data sort of like:

Byte 0 - how many IDS were in list, 
Byte 1 - How many were retrieved
Bytes 2-3 Data length for items...(N)
Followed by data: 
4 - ID element 1
5 - Err
6-(6+N-1) data for element

6+N - ID element 2

Will hopefully test later today.
...

[OpusK]

@KurtE

I created and used an API to change the buffer as shown below, but in the end, it seemed better to manage it as a separate class to support querying on received data. (Changing the buffer, there was no problem communicating.)

typedef struct InfoSyncBulkInst{
  uint8_t* p_packet_buf;
  uint16_t packet_buf_capacity;
  uint16_t packet_length;
  uint8_t id_cnt;
  bool is_complete_packet;
  uint16_t addr_len;
} InfoSyncBulkInst_t;

bool beginSyncRead(uint16_t addr, uint16_t addr_len, InfoSyncBulkInst_t *p_sync_info = nullptr);
bool addSyncReadID(uint8_t id, InfoSyncBulkInst_t *p_sync_info = nullptr, bool flag_end_add = false);
uint16_t sendSyncRead(uint8_t *p_recv_buf, uint16_t recv_buf_capacity, InfoSyncBulkInst_t *p_sync_info = nullptr);


bool beginSyncWrite(uint16_t addr, uint16_t addr_len, InfoSyncBulkInst_t *p_sync_info = nullptr);
bool addSyncWriteData(uint8_t id, uint8_t *p_data, InfoSyncBulkInst_t *p_sync_info = nullptr, bool flag_end_add = false);
bool sendSyncWrite(InfoSyncBulkInst_t *p_sync_info = nullptr);

Below is a simple example.
Of course, I'm still looking for a better way.

• sync_read_write_raw.ino

So far I have not put in any of the set Buffers, but did add some query functions to extract the data retrieved from the buffer. Currently I have the buffer encoded with data sort of like:
Byte 0 - how many IDS were in list,
Byte 1 - How many were retrieved
Bytes 2-3 Data length for items...(N)
Followed by data:
4 - ID element 1
5 - Err
6-(6+N-1) data for element
6+N - ID element 2

I would like your suggestions too.

[KurtE]

Hi Again, Will take a look at the above.

I just had a chance to get my earlier sketch to work, with a C version of SyncRead. Pushed up current stuff, will take a look in the next day or two at your newer stuff.

Kurt

[KurtE]

@OpusK ,

I synced up to your branch again and took a look at your example, which looks like you are getting there.

A few comments.

Not sure if better on addSyncReadID to have the 3rd parameter or simply have another method, like
endSetupSyncRead()... - probably can find better name. But what you have works.

in the method: sendSyncRead(), you are assuming that all of the servos in the list will respond and in the order in which they are in the list. But what happens if one of the servos does not respond?
i.e suppose you ask for servos 1, 2, 3, 4, 5, 6
and servo ids 3 and 5 does not respond? You loop 6 types waiting for message:

for(uint8_t i=0; i<p_info->id_cnt; i++)
{
  if(rxStatusPacket(&p_recv_buf[i*p_info->addr_len], 
  recv_buf_capacity-i*p_info->addr_len, 3) != nullptr){
    recv_cnt++;
  }
}

The data in your buffers you passed in , will probably write in the first 4 elements of it, for servos: 1, 2, 4, 6

And your test app will assume that for example index 2 is for Servo #3, and there is no way to verify which one it is actually for? As the loop above does not check IDS. There are at least a few options here?

a) Save the IDs as part of the user array... So they can loop through and know which ID they are looking at...

b) Have the loop code above verify the ID, matches, the expected one, if not, move the data down to where it should be in the array to the index expected, and probably clear out the data for the one missed? Some error value?

Also again not sure if you wish to worry about, but you are also not keeping the ERR values from the received servos, so for example if a servo has hardware issue, like it detected a low voltage, you will not get that information through the Sync Read.

Also it will be interesting to see if all of the boards you wish to support all use little endian? If not examples like the one that assumes that the 10 bytes are stored in the same format as what your servos expect and/or receive could mess up.

[KurtE]

Quick message/suggestion:

In the SerialPortHandler

class SerialPortHandler : public DXLPortHandler
{
  public:
    SerialPortHandler(HardwareSerial& port, const int dir_pin = -1);

    virtual void begin() override;
    virtual void end() override;
    virtual int available(void) override;
    virtual int read() override;
    virtual size_t write(uint8_t) override;
    virtual size_t write(uint8_t *buf, size_t len) override;

    void begin(unsigned long baud);
    unsigned long getBaud() const;

  private:
    HardwareSerial& port_;
    const int dir_pin_;
    unsigned long baud_;
};

I think it might be beneficial if your new begin(baud) method was virtual.
That is:
virtual void begin(unsigned long baud);
As for example suppose I am using this for a Teensy (or other hardware that supports hardware half duplex). And as part of the SerialX.begin, I may need to then do other calls or muck with registers, in order to initialize that serial port into half duplex mode.

Sort of for example how you have code added in the default specific to OpenCM or OpenCR...

[OpusK]

Hi, @KurtE

I'm sorry for late reply.
First of all, I only applied changing the virtual function.

Can I leave an answer for the sync function later?

[KurtE]

@OpusK

Thanks, sounds good.

As for Sync functions and the like, no worry, no hurry...

[OpusK]

Hi, @KurtE

Not sure if better on addSyncReadID to have the 3rd parameter or simply have another method, like
endSetupSyncRead()... - probably can find better name. But what you have works.

I also worried about this a lot. As you comment I may need to modify the names of the functions somewhat.

beginSetupSyncRead
addSyncReadID
endSetupSyncRead
doSyncRead

in the method: sendSyncRead(), you are assuming that all of the servos in the list will respond and in the order in which they are in the list. But what happens if one of the servos does not respond?

Yes. This is my mistake
If a protocol does not receive a response from one ID, it will not receive a response from the rest of the IDs, so a break statement is added to prevent unnecessary work.

  for(uint8_t i=0; i<p_info->id_cnt; i++)
  {
    if(rxStatusPacket(&p_recv_buf[i*p_info->addr_len], 
    recv_buf_capacity-i*p_info->addr_len, 3) != nullptr){
      recv_cnt++;
    }else{
      break;
    }
  }

And your test app will assume that for example index 2 is for Servo #3, and there is no way to verify which one it is actually for? As the loop above does not check IDS. There are at least a few options here?

In fact, this example assumes that the user enters the ID correctly.
Because it's a raw API, I've planned to do that in a separate class like yours.

By the way, when I try to create such a class, do I need to create these raw APIs?
If I think about it now, it doesn't seem necessary.
It would be better to create a separate class like yours with the functions below.

    // raw APIs
    bool txInstPacket(uint8_t id, uint8_t inst_idx, uint8_t *p_param, uint16_t param_len);
    const InfoToParseDXLPacket_t* rxStatusPacket(uint8_t* p_param_buf, uint16_t param_buf_cap, uint32_t timeout_ms = 10);

Also again not sure if you wish to worry about, but you are also not keeping the ERR values from the received servos, so for example if a servo has hardware issue, like it detected a low voltage, you will not get that information through the Sync Read.

The same seems to be the case.
Rather, it seems that the form of the function I created earlier met these parts. (There was a waste of memory.)
Perhaps this is better if we manage it in a Class form similar to yours.

Also it will be interesting to see if all of the boards you wish to support all use little endian? If not examples like the one that assumes that the 10 bytes are stored in the same format as what your servos expect and/or receive could mess up.

Currently, I am writing only a little endian. However, I think it's a good idea to allow endian users to choose and use it in the future.
Endianness is about buffering, so we tried to consider it after the rest of the functionality had stabilized.
Is there any code structure you think about this?

+Update:
I've modified the function regarding the return of error.
However, to make it easier to use, including ID checking, I think we need to create a separate class like yours.
When I create this class, I have to worry about whether it is more efficient to use the Sync function that I have made or to use only the raw API.

[OpusK]

Hi, @KurtE

I gave up several levels of rawAPI and changed back to the previous structure parameterized API. (Struct member changed)

Based on this, I plan to create a separate class and provide it like a class you create or a class method in the SDK and the workbench.

• sync_read_write_raw.ino
• bulk_read_write_raw.ino

+Update:
@KurtE, I think it's good to release this rawAPI first, and then create a separate issue to discuss classifying.
What is your opinion? And do you think this rawAPI is okay for release?

[OpusK]

@KurtE,

I simply created template classes. What do you think about this?

• class definitions in master.h
• sync_read_write.ino

[KurtE]

Hi @OpusK

Happy new years (actually the day before)...

I will take a look soon, been sidetracked with some other stuff, but hope to get back to it later this week or maybe early next.

[OpusK]

Hi, @KurtE

Happy new year! :)
My greeting is late!

I am always grateful for your contribution.

[OpusK]

Hi, @KurtE

I have released a modified version of this issue.
Please refer to 0.4.0 version.

But I didn't implement it in Class form (this still seems to be a concern). I used struct parameters just like before, splitting them up into new ways of dynamic allocation and legacy support. In other words, only raw API is currently supported.

[KurtE]

@OpusK

Will have to take a look and get back to porting my code over.

Always too many distractions ;) Most of which I create for myself.

[OpusK]

This issue has been closed as there weren't recent activities. Please feel free to reopen this thread if there's any opinion to throw. Thanks.