hlfshell / serial-synapse-socket

Given a synapse object, create a socket server for the device
0 stars 0 forks source link

Alter syntax of Command Responses to stay in line with CMDMessenger on arduino #1

Closed Macgyverman closed 7 years ago

Macgyverman commented 7 years ago

Love the project, but I had trouble getting it off the ground. Update messages follow the syntax of CMDMessenger on arduino ID,param1,param2, etc.

However, command responses expect a syntax of UUID,param1,param2, etc.

When using CMDMessenger's sendCMD function on arduino, the library responds as ID,param1,param2, etc.

If the syntax of the command response handler were altered to accept messages of the form ID,UUID,param1,param2, it would be possible to use built-in functions on the arduino CMDMessenger library to respond, instead of using a series of Serial.print

One would reply as sendCmdStart (ID); sendCmdArg (UUID); sendCmdArg (param1); sendCmdArg (param2); etc. sendCmdEnd ();

Compatibility could be maintained with legacy code by checking if the first parameter received by your script is an int or a UUID, and acting accordingly.

hlfshell commented 7 years ago

Thanks for the kind words. I would love to hear what you're using it for.

As for your requested change - I'll consider it.

I'm actually planning a large scale rewrite of synapse that will come one day when I find time. The overall idea is to rip the protocol out of it so it can be cmdmessenger, firmata, etc, and add extra features I'm toying with like device centric autoidentification and defining of functions instead of in the code. Because of this planned rewrite, I'm not so sure about making large changes like this in the meantime unless there's a strong reason for it.

Also, can't you just do

sendCmdStart(UUID);
sendCmdArg (param1);
sendCmdArg (param2);
etc.
sendCmdEnd ();

or does sendCmdStart limit to an int?

Also, is there anything else that's troubling you on getting up and running or is it just this syntax jarring you?

Macgyverman commented 7 years ago

Hi Keith,

Thanks for getting back to me so quickly.

The syntax got in my way a bit, but mostly because one of the examples was missing a Serial.print(‘,’) and because the cmdMessenger.readIntArg doesn’t appear to exist in the current version of CMDMessenger.h. It’s either readInt16Arg or readInt32Arg or the generic .readBinArg();

void setLED(){ char* uuid = cmdMessenger.readStringArg(); int ledStatus = cmdMessenger.readIntArg();

digitalWrite(LED_PIN, ledStatus);

Serial.print(uuid);
Serial.print(',');
Serial.println(ledStatus);

}

The example would timeout, and since there is no error handling in the example, it took me a few minutes to track everything down by reading your source code. Nothing major…the code is very well written! Thanks for publishing the library.

According to CMDMessenger.h, you can’t do sendCmdStart(UUID) as it expects a single byte as the parameter.

Thanks again, you’ve pushed me to use node.js on a Raspberry Pi to control my Arduino instead of relying on Python (whose syntax I can’t stand)

Background on my project : I’ve got a hot tub which has no intelligence (restrict heating to cheaper electricity hours, no alerts for power failures, no alters for water problems etc.) I’ve hacked together overriding the controller with an Arduino. I’m using a Pi to log data, send alerts via email and control the Arduino. I’ve been meaning to publish it on my blog (pierresarazin.com http://pierresarazin.com/) but the shared server got hacked and I just yesterday got my site back online.

Regards,

Pierre

On Jan 9, 2017, at 11:55 AM, Keith Chester notifications@github.com wrote:

Thanks for the kind words. I would love to hear what you're using it for.

As for your requested change - I'll consider it.

I'm actually planning a large scale rewrite of synapse that will come one day when I find time. The overall idea is to rip the protocol out of it so it can be cmdmessenger, firmata, etc, and add extra features I'm toying with like device centric autoidentification and defining of functions instead of in the code. Because of this planned rewrite, I'm not so sure about making large changes like this in the meantime unless there's a strong reason for it.

Also, can't you just do

sendCmdStart(UUID); sendCmdArg (param1); sendCmdArg (param2); etc. sendCmdEnd (); or does sendCmdStart limit to an int?

Also, is there anything else that's troubling you on getting up and running or is it just this syntax jarring you?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hlfshell/serial-synapse-socket/issues/1#issuecomment-271339554, or mute the thread https://github.com/notifications/unsubscribe-auth/AE-0HUA0LxPBIYq0mH3oKOIX5id-mI38ks5rQmaUgaJpZM4Ld5X0.

hlfshell commented 7 years ago

I'll update the documentation to switch out readIntArg and add in the missing ",", so thanks for point that out. Documentation is important but hard to self proofread sometimes, so I appreciate any and all corrections people can find.

For your kind of project I would also recommend looking into MQTT - that seems to be a great way to orchestrate IoT objects like your hottub. I've created, but not yet published, an example of mapping UDP wifi bulbs to MQTT here. This would allow you to hook up the hot tub to the MQTT server with no additional features, and then write management scripts nad loggers as separate systems.

Either way, good luck on your project and let me know if you find any other issues!

Macgyverman commented 7 years ago

Excellent!

Thanks for the information.

Regards,

Pierre

On Jan 9, 2017, at 1:33 PM, Keith Chester notifications@github.com wrote:

I'll update the documentation to switch out readIntArg and add in the missing ",", so thanks for point that out. Documentation is important but hard to self proofread sometimes, so I appreciate any and all corrections people can find.

For your kind of project I would also recommend looking into MQTT - that seems to be a great way to orchestrate IoT objects like your hottub. I've created, but not yet published, an example of mapping UDP wifi bulbs to MQTT here https://github.com/hlfshell/lifx-mqtt. This would allow you to hook up the hot tub to the MQTT server with no additional features, and then write management scripts nad loggers as separate systems.

Either way, good luck on your project and let me know if you find any other issues!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hlfshell/serial-synapse-socket/issues/1#issuecomment-271365324, or mute the thread https://github.com/notifications/unsubscribe-auth/AE-0Hfs9W8smOnx5kla6CWzXj1_iqlcrks5rQn1egaJpZM4Ld5X0.

Macgyverman commented 7 years ago

Hi Keith,

Thanks again for the library. I haven`t had a chance to play with it in the last months. I was working on it today and had a bug I couldn’t figure out. Messages were being sent to the Arduino, but responses from update handlers were being ignored.

The cause was partly my lack of understanding of how things work and partly the issue we discussed in January : the addition of UUID without including the command ID breaks the workflow in CMDMessenger.

1 - My lack of understanding : Using parsers to split up commands to trigger the data event requires commands to be separated by \r\n. I had configured it to separate commands by ; (the default separator for CMDMessenger). Since the parser truncates the separator, using ; would properly trigger the data event, but then render the message invalid, since it was then missing the ; CMDMessenger includes the ability to append \r\n after the message separator, but this is not enabled by default (and not in your example). One needs to explicitly call CMDMessenger::printLfCr(true) after creating the object in C++.

2 - After I solved #1, messages were still being ignored. I sniffed the commands and found that \r\n was not being appended. Even if this feature is enabled, the \r\n is not printed unless the user explicitly calls CMDMessenger::sendCmdStart(commandId). However, since commandId is a byte, we can’t call it, as serial-synapse expects the first part of a message to be the uuid.

Workflow Serial synapse expects: Serial.print(uuid); messenger.sendCmdArg(foo); messenger.sendCmdEnd();

Required workflow to properly trigger the sendCmdEnd in CMDMessenger : messenger.sendCmdStart(someIDNumber); messenger.sendCmdArg(uuid); messenger.sendCmdArg(foo); messenger.sendCmdEnd();

Actually, sendCmdStart needs to be called for a bunch of other reasons (it sets flags etc.) All flags it sets are private, and therefore one can’t simply create a derived SerialSynapseComaptibleMessenger class (I tried, but it became a huge hack).

Here is the workaround I implemented in your code. It’s roughly what we discussed in January, but it’s quite simple and will maintain compatibility with your old code :

Replace : var identifier = msg.split(",")[0]; if(self._waitingResponse[identifier]) return self._handleCommandResponse(identifier, msg); else if(self._updateHandlers[identifier]) return self._handleUpdateMsg(identifier, msg); With :

      //Scan the 1st two args to see if there is a uuid we know of
        var tokens=msg.split(",");
        for(var offset=0;offset<2 && tokens.length>0;offset++){
            msg=tokens.join(",”); //Rebuild the possibly shortened message
    var identifier = tokens.shift(); //extract either the uuid or the commandId, if we get a commandId, the next calls fail and we’ll move on to parameter 2 for a uuid instead
          if(self._waitingResponse[identifier]) return self._handleCommandResponse(identifier, msg);
          else if(self._updateHandlers[identifier]) return self._handleUpdateMsg(identifier, msg);
        }

Basically, it will now allow your library to handle messages in the form commandId,uuid,param1,param2; While maintaining compatibility with your original code uuid,param1,param2;

This will allow us to properly use sendCmdStart to set the flags and properly trigger sendCmdEnd

Regards,

Pierre

On Jan 9, 2017, at 2:50 PM, Pierre Sarazin pierre.sarazin@gmail.com wrote:

Excellent!

Thanks for the information.

Regards,

Pierre

On Jan 9, 2017, at 1:33 PM, Keith Chester <notifications@github.com mailto:notifications@github.com> wrote:

I'll update the documentation to switch out readIntArg and add in the missing ",", so thanks for point that out. Documentation is important but hard to self proofread sometimes, so I appreciate any and all corrections people can find.

For your kind of project I would also recommend looking into MQTT - that seems to be a great way to orchestrate IoT objects like your hottub. I've created, but not yet published, an example of mapping UDP wifi bulbs to MQTT here https://github.com/hlfshell/lifx-mqtt. This would allow you to hook up the hot tub to the MQTT server with no additional features, and then write management scripts nad loggers as separate systems.

Either way, good luck on your project and let me know if you find any other issues!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hlfshell/serial-synapse-socket/issues/1#issuecomment-271365324, or mute the thread https://github.com/notifications/unsubscribe-auth/AE-0Hfs9W8smOnx5kla6CWzXj1_iqlcrks5rQn1egaJpZM4Ld5X0.

hlfshell commented 7 years ago

Hi Pierre,

I'm not sure I understand the base problem you're having - are you trying to get update handlers to trigger from an incoming message? Or is the problem entirely around trying to send back a raw byte stream?

Also, is this more of a serial-synapse problem or is the problem entirely localized to the socket extension?