Hey guys
 
I was giving some thinking about how to implement the MrlMessage class and would like to open a discussion so we are all on the same wave.
 
The goal of MrlMessage class will be to
 
isolate the Serial access from the rest of the code
make it easier for maintenance if we need to change message structure
make it possible to implement other communication way, like using WiFi on esp8266,  I2C protocol etc
take care of the details
format the message (MAGIC_NUMBER, size etc)
I could think of many way to implement that class, but my prefered way would be a container-like class that look this way
 
class MrlMessage{
  MrlMessage(int MSG_TYPE);
  void addData(int dataSize, int[] data);
  void addData(int data);
  void addData(byte data);
  void addData(long data);
  void addData(String data);
  ...
  void sendMsg();
}
I think this offer the best flexibility to add new MSG_TYPE and an easy usage as you don't need to care about formating, bulding a array or converting the data to bytes as everything will be take care in the class
 
Let me know of what you think or see about the MrlMessage class implementation
 
Christian
 

GroG

8 years 4 months ago

I agree with the importance of the goals you stated Christian.

Isolating the Serial would be great..

When developing new classes I like to look at how it would be used ....
So this :
 
void publishVersion() {
  Serial.write(MAGIC_NUMBER);
  Serial.write(2); // size
  Serial.write(PUBLISH_VERSION);
  Serial.write((byte)MRLCOMM_VERSION);
  Serial.flush();
}
 
would become this ?:
 
void publishVersion() {
  MrlMessage msg(PUBLISH_VERSION);
  msg.addData(MRLCOMM_VERSION);
  msg.sendMsg();
}
 
Which I think is an improvement.
There are a few details which need to be considered.
How will the dynamic data be buffered in memory before writing to Serial in sendMsg()?  
Linked List seems like it would be approriate..  
You would want to be sure to fully deallocate it if you implement it this way, but don't dealloc the global Serial ;)
 
 
PS.  One suggestion Message is pretty consistently abrieviated in MRL .. So I'd suggest MrlMsg for a class name.
 
Cool !

yes, that's exactly how I want to use it.

 

for the data buffering before sending to serial, I have think about using the linked list, but still looking for other solutions.

Excellent!  Yes,  I think this is exactly the way we should go.   Thanks for your time and contributions Calamity!  

For messages that have a payload, we should support another constructor that takes a linked lists of ints and the message type.  I think that's a good idea.  Alternatively, we could pass in the payload to the MrlMsg class as an int[]   ...  

Ultimately, I think we need to avoid "computing" the "size" when sending the message.  

Perhaps it'd be easier to pass in an array for the payload of the message, than a linked list.  (Linked list , we'll have to make sure to de-allocate the memory properly...)

All great stuff!  

 

GroG

8 years 4 months ago

Ya .. this will be nice .. I see lots of code going away, abstraction of a connection (a wonderful thing), and single point communication & standardization of serialization ...

a concrete example:

Instead of this :

// send the average loop timing.
publishStatus(avgTiming, getFreeRam());
 
void publishStatus(unsigned long loadTime, int freeMemory) {
  Serial.write(MAGIC_NUMBER);
  Serial.write(7); // size 1 FN + 4 bytes of unsigned long
  Serial.write(PUBLISH_STATUS);
 
  // write the long value out
  Serial.write((byte)(loadTime >> 24));
  Serial.write((byte)(loadTime >> 16));
  Serial.write((byte)(loadTime >> 8));
  Serial.write((byte)loadTime & 0xff);
 
  // write the int value out
  Serial.write((byte)(freeMemory >> 8));
  Serial.write((byte)freeMemory & 0xff);
 
  Serial.flush();
}

we could do this :

// send the average loop timing.
MrlMsg msg(PUBLISH_STATUS);
msg.addData(avgTiming);
msg.addData(getFreeRam());
msg.sendMsg();
 
 
I would think the linked list of bytes could be the internal representation (ints really)... 
But as you see, addData(unsigned long) could have a single place of conversion like the above
// write long value out
which is msb ..
additionally all variable length types (String, int[], .. other) would write their size out in the first byte..
 
 

I add a first draft of MrlMsg class on cloud 9

still have some work to do on it, but it's worky with all the Publish type we had so far except the one in the I2C device (I can't test that one yet) 

I had some trouble with the overload and it's still need some cleaning.

int is often use to hold byte data so to avoid confusion I made

addData(int) -> cast the value to byte

addData16(int) -> send the 16bits of the int

 

 

Great progress calamity ...

I like you are really working on this .. great ideas !

Some suggestions :

  • Whenever you see duplicate code in overloaded methods, you should call one method from the other since the encoding is identical .. e.g.
      void addData(long data) {
        dataBuffer.add((unsigned long) data);
      }
      void addData(unsigned long data) {
        dataBuffer.add((byte)((data >> 24) & 0xFF));
        dataBuffer.add((byte)((data >> 16) & 0xFF));
        dataBuffer.add((byte)((data >> 8) & 0xFF));
        dataBuffer.add((byte)(data & 0xFF));
      }

    But I wouldn't do that either .. start with methods which are needed, vs trying to implement every type
     

  • You should only need the Linked List - dataBuffer you don't need strBuffer .. "Strings" should be encoded with the size like the following

    void addData(String &data) {
        
               dataBuffer.add(data.length())
               for (int i = 0; i < data.length(); ++i){
                             dataBuffer.add(data[i]);
                 }
     

thanks for the suggestion

for the overloaded methods, that's what I mean when I said it need some cleaning. I just throw in the overload methods as I need them when I change the publish method on my local file.

for the Strings overload, that's how I want to do it, but for some reason my try with it have failed. So I went with another solution instead of digging on it. But it's my intention to get rid of the strBuffer.

Hammer things on, then paint and wax. 

by the way, those are the message formatting it can support so far

MN | SIZE | TYPE | DATA                             (publishVersion, publishStatus, 
                                                    publishError, publishCommandAck, 
                                                    publishBoardInfo)

MN | SIZE | TYPE | DATA | STRING_DATA               (publishError)

MN | SIZE | TYPE | STRING_DATA                      (publishDebug)

MN | SIZE | TYPE | ID | DATA_SIZE | DATA            (publishAttachedDevice)

MN | SIZE | TYPE | ID | I2ADRESS | DATA_SIZE | DATA (I2C)
 

      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
nice description calamity ..


Your visualization shows  MN |  SIZE | TYPE   .. a standard msg header - as its handled in Arduino.sendMsg


The data which follows should be handled on a per C++ type basis ..  which is what you've done with the addData of different types..


for example a msg "could" be


MN | SIZE | TYPE | STRING_DATA ... | INT | LONG | STRING_DATA ... | INT


We leave it to Arduino to decode the message based on its TYPE .. so it knows the placement of the types ..  only "challenge" occurs at variable length types  .. int[] & String .. so if both sides know that the size of a variable type comes with size first .. eg.


	MN | SIZE | TYPE | STR_SIZE | STRING_DATA ... | INT | LONG | STR_SIZE | STRING_DATA ... | INT


it will be decoded properly and handle any "set" of types 


Perhaps we could fully convert to C++ too .. because .. then CDT formatting might work..


 



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
rework a  bit the MrlMsg class


for  int[] and String, I add an inclDataSize variable to output or not the data size


so msg.addData(string, true) will output STR_SIZE | STRING_DATA


msg.addData(string, false) or addData(string) will only output the STRING_DATA


 


I also add countData() and addDataCount()


so by example


MrlMsg msg(PUBLISH_SOMETHING, deviceID);
msg.addData(string,true)
msg.addData(int)
msg.countData();
while(node != NULL){ //looping in a linkedList of sensor 
  if(value != lastValue){
    msg.addData(int);
    msg.addData(long);
  }
}
msg.addDataCount();
msg.sendMsg();
will output: MN | SIZE | TYPE | ID | STR_SIZE | STR_DATA | INT | DATA_COUNT | INT1 | LONG1 ... | INTn | LONGn


 


I think with that we can make any message pattern we need without modifying MrlMsg class


 



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
Great updates Calamity !,


have some questions ....  Nice to see MrlMsg take shape and Serial access get isolated.


What is the purpose of the following ?



	  // enabled the data counter



	  void countData(){



	    dataCountEnabled = true;



	    dataSizeCount = 0;



	    dataSizePos = dataBuffer.size();



	  }



	 



	I like your idea of having a static next device counter in the Device too - it should be incremented there too.



	 



	I don't like statics in C methods e.g.



	 





		bool getCommand() {



		  static int byteCount;



		  static int msgSize;



		 



		another reason we should turn it all into C++ I think 




I ran ServoTest with the latest on Cloud9 - looks worky ! :)



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
the countData() method allow MrlMsg to set the size of a subgroup of data when not passing an array to MrlMsg. 


so it make it possible to MrlMsg to output something like STR_SIZE | STRING | DATA_SIZE | INT...


in most case we know beforehand what the DATA_SIZE is in arduino, but there may be some case that we don't


by example DigitalPinArray. In the previous version, the value of the digital pin where send when the value change.


so the update method of digitalPinArray will look like


update(){
  MrlMsg msg(PUBLISH_SENSOR_DATA,ID)
  msg.addData(...)
  msg.countData()
  foreach pin{
    if (pinvalue != lastValue) {
      msg.addData(pinAdress)
      msg.addData16(pinValue)
    }
  }
  msg.addDataCount()
  msg.sendMsg()
}
will output MN | PUBLISH_SENSOR_DATA | ID | ... | DATA_SIZE | DATA


 


 


I'm not a fan too of statics in C method, but i'm even less a fan of globals that are used in only one method and that are named with a common label



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
I'm glad we agree on statics in C++ classes can be a good thing :)


I don't think countData is necessary ...


A heads up : I'm starting implementation of Java-Land PinArray 

	I think we do not need 2 different types (digital vs analog) ..  these are just details on how we want the data read, not a detail of the Pin itself (ie you can do meaningful digitalReads to analog pins)


Ultimately, the Javaland decoding based on Device.type needs to match what is being sent .. and I think we can probably figure out a way to send it with one of your very cool addData(type) without the need of countData


We'll see ... :D


 


Oh we need a little coordination to wrap the last C only parts up .. you want me to do that ? or do you want to  create the class MrlComm ? ;)



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
I can work on the MrlComm class so you can use most of your brain juice for the javaland in wich i'm terrible


So I will start wrapping the remaining into a nice package


From there it will be easier to see what need to be done



      
    
          
  • 
      

    

  











    

  

    

      

  


    

  

  

    

      
GroG

      
8 years 4 months ago

                

    

              
        

        
            
            
That last merge was a little rough .. Please verify NeoPixel .. ServoTest is worky for me..

	I renamed your NeopixelGui.js to NeoPixelGui.js hopefully it all worky too



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
well look like it got wrong after all


when I look this morning all was ok, but now the Neopixel have revert to an old version, like if you have overwrite my push with version 1505


I still have the last version of neopixel if that help fixing it


 



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
Sorry .. yes ...  I think I might have borked it when I got a dirty index from git .. trying to manage things from local, github, cloud-9 & pull requests can get a bit overwelming ...


I sent you a member invite so you can check in directly without a pull request ..


Welcome Calamity !





      
    
          
  • 
      

    

  

















    

  

    

      

  


    

  

  

    

      
GroG

      
8 years 4 months ago

                

    

              
        

        
            
            
Speaking of help.


The cloud9 version of MrlComm looks like its whack ... (very old)


The GitHub develop/head version looks "better" ..


I did a git pull on cloud9 - but I suspect someone has modified the file - because they are not in sync... and it does not update


So can you look at develop/head with what you have locally - and verify its the latest.


When / If you verify .. I will overwrite (or stash) what is on cloud 9 with develop/head


Thanks !



      
    
          
  • 
      

    

  








    

  

    

      

  


    

  

  

    
    

              
        

        
            
            
I'm seeing only formatting difference and no code change


I'm working on turning MRLComm into a library type code.


that's imply some change...


  • 
		Moving arduino code to resource/Arduino/MRLComm folder

    • 
				that's  a requirement to use ArduinoIDE on the resource path without having to copy the file somewhere else on the computer system
      • 

    • 

  • 
		Exploding MRLComm.c

    • 
				an ArduinoMsgCodec.h file for the auto-generated part
      • 

    • 
				an class.h file for each of the class we have build
      • 

    • 

  • 
		renaming MRLComm.c to MRLComm.ino
    • 

  • 
		Add a method in Arduino.java to upload the sketch to arduino

    • 
				it use already existing org.myrobotlab.arduino.AduinoUtils
      • 

    • 
				user need to tell where ArduinoIde is installed but then it's easy to upload the sketch to arduino
      • 

    • 

 



      
    
          
  •