Dynamixel sdk playing with sync read write

[KurtE]

Put up this Pull Request, that tries to reduce or eliminate any memory allocations done as part of group_sync_read or group_sync_write.

They both create a single buffer, that holds all of the data. It also allows the using program to statically define this buffer and tell the code to use their buffer instead. This allows programs to get a better understanding of exactly how much memory they are using.

Also it removes all of the calls to heap manager. (Except to allocate and free the one buffer)

I also added a method to sync write that worked like Sync Read, such that you could pass in an uint32 value and size of your field and it would do the appropriate packing.

So far I have only done these two objects. Something similar could be done for the Bulk read and write.

If this level of change looks valid to you, I can also migrate this code over to both the SDK project as well as OpenCM.

This pull also includes an extended Sync Read and write example that the Sync Write updates both position and velocity and reads in position, velocity and force assuming XL430... servos.

I may have also semi accidentally checked in my Dynamixel workbench test (find servos) update that made it more Arduino ish, like using Serial.print(...) function instead of concatenating strings. Also moved code to loop, to allow it to run again. And maybe a debug message I had which printed out the size of the workbench object.

Let me know if you like these changes or if you think parts of it should change.

[kijongGil]

Hi @KurtE :) Thanks to your contribution! I'll talk to you after reflecting your PR in DynamixelSDK and testing it this week. We will receive your PR after that.

Thanks, Gilbert

[KurtE]

@kijongGil,

Wondering about the differences in the sets of files. (OpenCR/OpenCM) versus Dynamixel_SDK project.

There are some simple differences. Things like copyright messages and the like.

The main functionality difference I see is with the Protocol(1/2)PacketHandler::readRX, the main SDK version you pass in an ID to verify that the packet was from an ID and the Open... versions do not and simply grab the next message.

[kijongGil]

Originally, the two codes should be the same. But, we did not update the DynamixelSDK in OpenCR, so the difference between main SDK and OpenCR version was increasing. I feel I need to synchronize the two versions. So I will be working on it next year.

[OpusK]

@KurtE ,

SDK has been updated with 1.4.0 release. Would you like to check it out?

[KurtE]

Hi @OpusK and @kijongGil ,

As I mentioned in other one. Sort of tied up. We were without power for about 24 hours... Luckily it is back...

I tried compiling one of the Examples I did for testing the Sync read and write and it currently does not compile.

That is lines like:

  dxl_wb.addSyncWrite("Goal_Position");
  dxl_wb.addSyncRead("Present_Current");
  dxl_wb.addSyncRead("Present_Position");
  dxl_wb.addSyncRead("Present_Velocity");

My guess is these function names were updated, probably to:

addSyncReadHandler
addSyncWriteHandler

Note: if so the file keywords.txt was not updated to the new method names...

[KurtE]

Hi again and happy Holidays @OpusK and @kijongGil ,

Actually looks Dynamixel Workbench API for SyncRead and SyncWrite is more significantly changed. Need to figure out the new proper semantics. That is before I had a quick and dirty test program, that added the one SyncWriteHandler and 3 SyncReadHandler. It only called the addSyncWriteHandler and it was assumed that it applied to all pinged servos...

But the new one you pass in an address. Do you only call this once? And the ID is just so you know can map it to control table to then lookup the String and convert to address? Or do you call this for each ID you wish to do the SyncRead (or write) on?

The new syncRead and syncWrite methods, have an index you pass in? Where do you get this index?
Do you expect that the callers to do something like:

  result = dxl_wb.addSyncWriteHandler(dxl_id[0], "Goal_Position", &log);
  goal_position_index = getTheNumberOfSyncWriteHandler() - 1; 

But oops I see that this PR/Issue is about the SDK not the workbench... So maybe should remove these comments?

NOW back to this PR request...

As for this PR, I think it is more or less toast... That is it looks like the current stuff still uses all of the heavy weight stuff: Example look at the start of GroupSyncRead

class WINDECLSPEC GroupSyncRead
{
 private:
  PortHandler    *port_;

  PacketHandler  *ph_;

  std::vector<uint8_t>            id_list_;
  std::map<uint8_t, uint8_t *>    data_list_;  // <id, data>
  std::map<uint8_t, uint8_t *>    error_list_; // <id, error>

The code is still doing lots of new/delete and the like. Likewise:

class WINDECLSPEC GroupSyncWrite
{
 private:
  PortHandler    *port_;
  PacketHandler  *ph_;

  std::vector<uint8_t>            id_list_;
  std::map<uint8_t, uint8_t* >    data_list_; // <id, data>

So again using vector and map which will again do lots of New/delete calls. It also has duplicated info? That is if you look up the id 3 in the id_list_ and it comes back as index 2. It is again probably safe to say that it will be in index 2 in data_list_. But you will again look through the duplicated data to find it again..

Again if interested, the PR could be redone again to the current code base. Not sure what all changed. But I know that none of the memory reductions appears to be in the current stuff that was released.

Kurt

[OpusK]

Hi, @KurtE. Happy Holidays!!

I tried compiling one of the Examples I did for testing the Sync read and write and it currently does not compile.

Is it compiled in OpenCR 1.4.1? In which example did you get this error?

[KurtE]

Hi @OpusK and again Happy Holidays,

Sorry I may have used poor choice of words, by saying Example. What I tested was my own test program based on the earlier release of OpenCM/OpenCR boards.

The issue is more of, what are your policy on compatibility between releases? That is a customer develops an application, using Dynamixel SDK or Dynamixel Workbench and then there is a new release of the Arduino boards for OpenCM or OpenCR, should their existing programs continue to compile and run. And if not, should the customer then have to add #ifdef code in depending on release number (assuming there is a defined value to check), so that their code will build for both the old and the new as maybe some of their customers may not have updated yet to new release...

I tried asking that over on Dynamixel Workbench project in an issue which is now closed: https://github.com/ROBOTIS-GIT/dynamixel-workbench/issues/206

Again this probably should be split off to own issue/PR, luckily I don't think it impacts most of my own stuff as I use the SDK and not workbench.

As for this PR, again looks like none of the memory usage things have been included in this release. Also wonder now if these changes are wanted if I would need to create a new PR, and try to fold the changes from this branch into the current code base...

[OpusK]

Hi, @KurtE,

Sorry for late reply and any inconvenience.

The issue is more of, what are your policy on compatibility between releases?

There seems to be a clear policy on this and some things that are not. I will discuss this with related parties.

[KurtE]

Not a problem - Currently I am busy doing some stuff for new Beta Teensy board 4.0...

Will get back to robotics stuff soon

[LucidOne]

Our team is also interested in seeing improved sync read/write performance merged.

Semantic Versioning the SDK would probably help.