search

arachnidlabs / reload-pro

Schematics and code for the Re:load Pro
http://www.arachnidlabs.com/reload-pro/
Apache License 2.0
98 stars 32 forks source link

Enhanced serial communication #92

Open knarfS opened 8 years ago

knarfS commented 8 years ago
Arachnid commented 8 years ago

Thanks for the contribution!

While I'm onboard with most of the changes, changing the responses for existing commands from "ok" to the command name will break any existing code that interfaces with the R:LP, which I really don't want to do.

If you're happy to drop those changes, and squash them into a single commit (or one per feature, rather than one per file), I'd be happy to accept your PR.

knarfS commented 8 years ago

Hi Nick.

Yes I'm aware of that problem, but the actual responses makes it very hard for a client application to handle the serial communication. This is due to the fact, that the communication is both blocking (wait for the response of a command, like set X) and non-blocking (wait for non requested responses like set, on, off, read, ...). But I will try to work around the fuzzy responses in my application :)

So you can change 'bl', 'cal', and 'reset' back to the 'ok' response. The response of the 'monitor' command is new and should be ok?

But I would vote for the changed responses of the 'on'/'off' commands. So the responses would be the same for the unrequested responses, like for the 'set' command or the 'read' command.

At the moment I'm working on a client for the RLP in Qt (Windows, Linux), with recording functionality, graphs (current, voltage, power, resistance, amp hour and watt hour over time, amp hour and watt hour), min-/max-values, etc. I'm hoping to release a first version in the next few weeks.

Arachnid commented 8 years ago

I agree that the current setup isn't ideal. What I'd really like to do is replace the whole communications protocol with something SCPI compatible. But in the meantime, I don't think it's safe to simply break the protocol for anyone else who's already using it.

Assuming it doesn't add too much overhead to the firmware size, you could add a command to switch on the new response format for existing commands. Different responses for new commands are absolutely okay.

knarfS commented 8 years ago

Yes, it took a few minutes to figure out how the command parsing in commands.h works :) SCPI would be nice, but pretty tight in memory.

What's about the 'on'/'off' response? Will you re-add the 'ok' response in comms.c? That would result in 2 responses ('on'/'off' and 'ok'). I'm fine with that behavior.

Arachnid commented 8 years ago

I think the existing behavour for existing commands - 'ok' only - needs to be preserved for compatibility. I realise this isn't ideal for an app, though it's not intractable, since 'ok' is only issued in response to interactive commands, you can listen until you receive that.

If you're happy to amend the PR so it doesn't break compatibility, and squash the commits down into one, I'm happy to accept the PR.