Feature/chiller integration #218
Conversation
…iller. Commands are assembled by hex command number and followed by a number of databits and the data itself. For get messages no data are transmitted.
…e communicates in hex code and have no terminator character. They have a fixed header consisting of a leading character, definition of lsb and msb order followed by a command represented by hex number. The header ends with the number of data bits that are send. The next N bits (as specified in the header) are data bits. It follows a checksum. Communication relies on first reading header, extracting number of data bytes, reading databytes and checksum and checking the full message. The class was tested and worked.
… now conversion from config file str to command is fixed
…not need to be in bytes. Handling of hex conversion to bytes happens in service now. Command from entity is now base_cmd + data, service checks the number of bytes and formats the command correctly. For get commands only the base_cmd is required, while for set now the data can be directly attached in hexstr format. This allows for easier implementation of the entities that also implment set or any other type.
…ring and not a numeric value, we use that for status strings
wcpettus
left a comment
wcpettus
left a comment
There was a problem hiding this comment.
I left a few comments throughout based on my quick impressions. Happy to discuss more.
|
|
||
| # do something here | ||
| decimal = 10.**(-int(result[0], 16)) | ||
| unit = self.units[int(result[1], 16)] |
There was a problem hiding this comment.
Unit is constant and does not change. We either can return f"{value} {unit}" as a string or we just return the value as numeric value. If I am right we can not have a tuple as payload. If its a string that contains the units as well, then you would need a calibration function that strips the unit again and makes it a float. I see pros and cons for both versions. As you may see I evaluate both, but just return the value. I had a version before where I had value and unit as a string.
| try: | ||
| data = self._send_commands(commands) | ||
| except Exception as err: | ||
| logger.critical(str(err)) |
There was a problem hiding this comment.
Following up on @wcpettus 's question about this method, this try/except structure seems to be where the differences are between this method and EthernetSCPIService's implementation.
@wcpettus: Is this structure maybe better than the original? If there's a first failure, we treat the second attempt in the same way regardless of what type of exception it was. Is there a reason we don't do a second try/except if the first exception was a socket.error?
@renereimann Another difference is that you don't attempt to call _reconnect() after the first failure, like the original implementation does. Is this due to a difference in how the Thermo Fisher devices work?
There was a problem hiding this comment.
There is no _reconnect in this implementation - all the socket connection is handled down in _send_commands (which is a separate question I have). Maybe I just answered my own question about why you need to override the inherited method.
The original argument was that a socket.error means the ethernet device needs to flush the socket connection (=reconnect). If it's a different kind of error, then I'm not sure what's going on and any troubleshooting is shooting in the dark. Mostly the difference is the error messages you get out - which makes it a fairly quick structure in the original.
There was a problem hiding this comment.
The _reconnect function as implemented for the SCPI device also tests for the cmd_at_reconnect, which is not implemented here. In principle thats also possible for the thermo fisher device (there is a "request acknowledge") but the structure is the same as a ThermoFisherHexGetEntity implements and the result is a version number defined in hex string. So there has to be additional wrapping in here.
| def bytes_to_ints(value): | ||
| return [byte for byte in value] | ||
|
|
||
| class EthernetThermoFisherService(Service): |
There was a problem hiding this comment.
I'm wondering if it would make sense to inherit from dripline.EthernetSCPIService and reimplement _send_commands() (we could think about restructuring EthernetSCPIService._send_commands() to use _assemble_cmd(), which would further simplify this class), and _listen(), plus adding the other necessary methods.
There was a problem hiding this comment.
If I revert back to use the _reconnect function I can inherit from EhternetSCPIService. I anyway have to overwrite the init due to different arguments that are needed but then only _send_commands() would need modifications.
nsoblath
left a comment
nsoblath
left a comment
There was a problem hiding this comment.
Thank you for the very thorough description of your feature and what it accomplishes with respect to communicating in this particular way with the Thermo Fisher devices. That was a great description of the PR.
| Entity.__init__(self, **kwargs) | ||
|
|
||
| @calibrate() | ||
| def on_get(self): |
There was a problem hiding this comment.
I have a question on the @calibrate qualifier on the on_get method. At first I tried to call the ThermoFischerHexGetEntity.on_get() function within the ThermoFischerNumericGetEntity.on_get(), since we first have to get the hex string as in the ThermoFischerHexGetEntity and then do further work on it. However when running this code, I got errors from the calibration method, which tried to do the calibration on the result of ThermoFischerHexGetEntity.on_get() instead of the ThermoFischerNumericGetEntity.on_get(). Because of that I fully rewrote this piece. Any suggestions if that can be simplified?
I modified the EthernetThermoFisherService to now inherit from EthernetSCPIService. This now uses the _reconnect method and no changes to send_to_device are needed. I make use of the EthernetSCPIService.__init__ . Also the acknowledge at reconnect is now tested. _send_commands is overwritten but simplified compared to earlier version, since the connection is now handled elsewere. The _listen method is not used.
|
I now rewrote the EthernetThermoFischerService which now inherits from EthernetSCPIService. I have tested the implementation with our device and things are working. |
nsoblath
left a comment
nsoblath
left a comment
There was a problem hiding this comment.
Nice reorganization. The main thing that I think needs to be changed is the way the arguments are handled in EthernetThermoFisherService.
| logger.debug(f"sending: {cmd}") | ||
| self.socket.send(cmd) | ||
| logger.debug("Wait for responds") | ||
| time.sleep(0.6) |
There was a problem hiding this comment.
Do we need the hard-coded sleep? If we call socket.recv() doesn't that effectively wait until something arrives on the socket?
| time.sleep(0.6) | ||
| logger.debug("Read header") | ||
| header = self.socket.recv(5) | ||
| nBytes = int(header[-1]) |
There was a problem hiding this comment.
Do we need any error handling after each recv() call? Ok if the answer is "no"; I just wanted to check that we're not missing something.
…vice. We check kwargs and add the defaults if they are not present
nsoblath
left a comment
nsoblath
left a comment
There was a problem hiding this comment.
Thanks for these updates, @renereimann
…We add a comment on how an alternative way to implement it could look like, but since we can not test it, we leave the current version.
Our chiller speaks some very special protocal, thus we implement speciallized Service and Entities.
The following classes are implemented
EthernetThermoFisherService implements the communication protocol with the device.
A message consists of a header + command + number of data bytes + data bytes + checksum as indicated in this image from the manual
The Service takes care of the lead_character, the lsb and msb address as well as byte counting and checksum calculation.
The communication is a half-duplex communication via a serial socket. The answer message follows the same structure as the request message where the command is repeated but the data bytes may be different. The resulting message is striped to data bytes and results are returned as hex-strings.
ThermoFisherGetHexEntity implements a get method. A cmd_str is interpreted as hex string. Some formating is needed since depending on the value the config file passes ints or strings. The resulting data from the device are send as hex-string. This entity can be used e.g. for status checking since the status is encoded in a bit-pattern.
ThermoFisherGetEntity implements a get method that returns a numeric value. All get commands on the thermo fisher device return three bytes, represented as hex-string. The first byte encodes the unit and a decimal qualifier. The second and third bytes represent a 2byte int. The resulting final value then is value*decimal qualifier in encoded units. The ThermoFisherGetEntity only returns the numeric value without returning the unit, however a lookup table is used on the fly.