I've been working on debugging various issues that people are having with the arduino service.  I've found that it's a bit overwhelming to read through so I took a crack at cleaning it up.  Below, I will share some notes about the coding convention that I've taken on with this refactor/code cleanup.

Design principals

  • Keep it simple
  • keep it clean
  • have good comments
  • avoid whitespace (2 spaces, no tabs)

MRLComm is an arduino sketch.  By loading this sketch on the arduino MRL can communicate with the arduino as a slave device.  This messaging happens over the serial port.  In general, the Arduino service in MRL will send a command to the serial port, the MRLComm code running on the arduino listens for that message.  When the MRLComm sees a message, it processes it.  

Arduino sketches are basically a c program with 2 methods "setup()" and "loop()".  

setup() gets called when the board powers on, or the serial port is connected.

loop() gets called over and over again, as it's name might suggest.  

The MRLComm sketch implements these two methods.

The setup() method in MRLComm is responisible for setting up the serial port, initializing the state of the pins on the arduino, and (now) sending it's current version number over the serial port to say it's there.

The loop() method has the basic flow:

  1. read from the serial port and try to get a command (if full command is ready process it)
    1. read all data available on the serial port
    2. see if a full message has been read return true
    3. side effect that the command buffer now contains a full message.
  2. If a full command is available
    1. process that command
    2. clear the command buffer and clean up memory
  3. update the servos
    1. this handles updating the position for servos that are sweeping
  4. update the sensors
    1. this method handles reading analog and digitial pin data and writing that back to the serial port.
  5. update internal timing and debugging information.

 

Each MRL message is mapped to a method with a similar name in MRLComm.  For example the GET_VERSION message is mapped to the method getVersion() in the arduino code.

Eventually, it would be nice to generate these methods in the arduino code auto-magically from java.  This is really an effort to organize the code in MRLComm better so that we can better debug & support it.

Much feedback welcome!

 

 

 

 

Mats

8 years 6 months ago

Hi

Thanks for explaining the cleanup. Sounds like a very good refactoring. 

I made some changes in MRLComm a few months ago, to add support for i2c. The changes I made to MRLComm are not the way I want them to be, because the code in MRLComm is device specific and that's not good at all. At that time I didn't make any changes to the Arduino service. I just used the methods in Arduino from the Adafruit16CServoDriver service. That's also not the way I want it to be. So I will start refactoring  Adafruit16CServoDriver, Arduino and MRLComm. 

I have waited to do this, because I didn't want to put anything at risk with Adruino and MRLComm during the Paris and San Fransisco Maker Faires.

But before I start to make any changes to Arduino or MRLComm, I just want to check that I understand the "generated" parts of Arduino and MRLComm. So I will describe the workprocess as I understand it now based on what Greg wrote in the shoutbox a few months ago. 

1. Create the new methods to be used in Arduino.java. Be careful about the naming conventions because the method names will be used to generate constants later in the process. For example a method name of servoAttach will generate a constant named SERVO_ATTACH and assign a value to it.

2. Find the ArduinoBindingsGenerator.java in org.myrobotlab.codec.serial. This program will generate three different files based on the method names in Arduino.java. Each of the methods in Arduino will be assigned a constant name and a constant value that will be used in the commnication between Arduino and MRLComm. If you have defined methods that don't need to be implemented in MRLComm, then add them to the "exclude.add" list  in ArduinoBindingsGenerator method generateDefinitions.

3. Execute the ArduinoBindingsGenerator. It will create three ArduinoMsgCodec<nn>.<type> files in the myrobotlab folder. To put the generated content into the correct place is a manual process. The content of the .ino file should be copied into MRLComm.c in resource.Arduino. The content of the .java file should be copied into Arduino.java in org.myrobotlab.service. The content of the .py file should be copied into  ArduinoMsgCodec.java in org.myrobotlab.codec.serial. Be careful to copy and replace the different parts to the right places in the code. 

4. Update the version numbers. MRLCOMM_VERSION in MRLComm.c should correspond to MRLCOMM_VERSION in ArduinoMsgCodec.

5. Implement the methods in Arduino and MRLComm.

6. Test until everything works.

7. Commit and push the changes.

Please check if I understand things right and correct me if I'm wrong.

/Mats

Hi Mats!

  I saw the adafruit methods in there.  I didn't change any of the structure or functionality of MRLComm in this refactor.  I was mostly concerned with just making the code more readable.  I hope it will become less scary for others to review, understand, and modify like this. 

  I believe, at this time, the only part of the MRLComm code that is generated is the #define statements at the top to make sure that the enum from the ArduinoMsgCodec are mapped properly to the MRLComm #define statements.

  One of the goals in my refactor was to make it so that every message type has it's own method in the arduino code.  I think that we could probably generate much more of the arduino code with it organized like this.  Now the setup and loop methods won't change when we add new features.  The only method that changes now would be the "processCommand()" method.  That will only add an additional entry to the switch statement and the associated function to call to process that method.  

  One other key design principal that I was trying to refactor towards is that anything that writes to the serial port should be standardized, I started with the convention that the MRLComm code will publish data, so there are methods for publishing any data back over the serial port where it will get read by the serial port and passed to the arduino service.

  Lastly,  I would really like to add some helper methods so that we can pass arbitrary strings as debug messages from MRLComm to the Arduino service.  I think having the ability to have some debug messages would go a long way to allow us to better how MRLComm responds in different situations.

Best,

  -Kevin

 

GroG

8 years 6 months ago

Great Progress Guys !

I think I pondered putting it in methods before.   
There is a silly amount of performance gain inlining, but definately not equal to the gain of readability.

With your improvements, more could be generated - as the complex "business code" is isolated from parsing and communication..  This is a Good Thing :)

Potentially the top half of the file could be generated - further simplifying future maintenance (Yay!)

  • Encapsulating MRLComm into a library - I've done this before, but ran into some form of memory leak.   It would be really nice as a library, where users could augment their sketches with MRL
  • Dual Binary & Textual protocol handled - I'm interested in being able to submit textual protocol similar to the MRLComm REST interface  e.g. /method/param0/param1...
    This would aid in easy debugging, and more transparent interactions - if the Communication protocol is generated, it makes it much more modular, and this modularity potentially supports different protocols without maintenance problems.