CommandEntity and FormatEntity with asteval supports#221
Conversation
…mand entity that we can use e.g. to degas our pressure gauges
|
As discussed at yesterdays call here is a config file that uses both new implemented Entities: |
| set_value_map (str||dict): inverse of calibration to map raw set value to value sent; either a dictionary or an asteval-interpretable string | ||
| extract_raw_regex (str): regular expression search pattern applied to get return. Must be constructed with an extraction group keyed with the name "value_raw" (ie r'(?P<value_raw>)' ) | ||
| ''' | ||
| super().__init__(**kwargs) |
There was a problem hiding this comment.
We can't use super().__init__() because that mechanism doesn't work with the pybind11-wrapped code. You should directly call FormatEntity.__init__(). Once super() isn't used for part of the class hierarchy, we have to not use it elsewhere.
| **kwargs): | ||
| ''' | ||
| Args: | ||
| get_str (str): sent verbatim in the event of on_get; if None, getting of endpoint is disabled |
There was a problem hiding this comment.
This list of arguments doesn't match what's in the __init__() function definition.
There was a problem hiding this comment.
I added the additional asteval_str in the documentation. I still keep the other arguments since they are propergated via **kwargs and are needed by the baseclass
There was a problem hiding this comment.
This seems like it invites version sheer between the docstring here and in FormatEntity - should maybe discuss convention.
| logger.debug(f"matches are: {matches.groupdict()}") | ||
| result = matches.groupdict()['value_raw'] | ||
|
|
||
| result = result.replace('\x00', '') |
There was a problem hiding this comment.
I removed it. This is not needed
|
|
||
| @calibrate() | ||
| def on_get(self): | ||
| if self._get_str is None: |
There was a problem hiding this comment.
Should this block (lines 42-54) be replaced by a call to FormatEntity.on_get()?
There was a problem hiding this comment.
Yes, I now use the FormatEntity.on_get function here
| __all__.append('CmdEntity') | ||
| class CmdEntity(Entity): | ||
| def __init__(self, cmd_str=None, **kwargs): | ||
| Entity.__init__(self, **kwargs) |
There was a problem hiding this comment.
Class and init function need documentation
There was a problem hiding this comment.
Added documentation
| logger.debug("Command function was successfully called") | ||
| if self.cmd_str is None: | ||
| raise ThrowReply('service_error', f"endpoint '{self.name}' does not support cmd") | ||
| return self.service.send_to_device([self.cmd_str]) |
There was a problem hiding this comment.
The call to Service.send_to_device() makes me think this is intended to be used with an EthernetSCPIService. That should be included in the documentation.
There was a problem hiding this comment.
Added that this is for EthernetSCPIService
…s already defined in FormatEntity in init and on_get function.
…e to apply the asteval function to the dict value not the dict itself.
|
Two thoughts/questions on the CmdEntity related to this PR, but beyond the scope:
|
wcpettus
left a comment
There was a problem hiding this comment.
If I post a review, do my other comments go from "Pending" to be visible?
|
|
||
| def cmd(self): | ||
| logger.debug("Command function was successfully called") | ||
| if self.cmd_str is None: |
There was a problem hiding this comment.
this placement feels odd - naively I would expect a CmdEntity which doesn't have a cmd_str provided to fail on init because it can't be useful (I would move this check to the init function and throw a ValueError if it's not provided, something like this https://github.com/driplineorg/dripline-python/blob/main/dripline/implementations/entity_endpoints.py#L41)
but maybe I'm missing an obvious use case
There was a problem hiding this comment.
Good point, I moved it to the constructor.
| **kwargs): | ||
| ''' | ||
| Args: | ||
| get_str (str): sent verbatim in the event of on_get; if None, getting of endpoint is disabled |
There was a problem hiding this comment.
This seems like it invites version sheer between the docstring here and in FormatEntity - should maybe discuss convention.
|
|
||
|
|
||
| __all__.append('CmdEntity') | ||
| class CmdEntity(Entity): |
There was a problem hiding this comment.
May want a larger discussion on organization of the Entity "zoo" we are creating. The CmdEntity may be harder to find here in the same file as the Asteval one since they don't have an explicit relation (other than the same device needing both, but that subtlety will be lost on users of other systems)
There was a problem hiding this comment.
I made a separate file for this. I agree that it should not go in the asteval_endpoint.py but we may easily end up with lots of files
|
|
||
|
|
||
| __all__.append('FormatEntityAsteval') | ||
| class FormatEntityAsteval(FormatEntity): |
There was a problem hiding this comment.
May want a style guide for naming - typically the base class (Entity here) goes last
Does that make this a FormatAstevalEntity or an AstevalFormatEntity?
There was a problem hiding this comment.
I changed to AstevalFormatEntity
…ormatEntityAsteval into AstevalFormatEntity, checking of input at constructor, improving doc strings
wcpettus
left a comment
There was a problem hiding this comment.
Looks good - we should keep an eye on CmdEntity, that may be worth elevating into dl-py codebase
| @@ -0,0 +1,31 @@ | |||
| import re # used for FormatEntity | |||
|
|
|||
| from dripline.core import calibrate, ThrowReply | |||
| @@ -0,0 +1,31 @@ | |||
| import re # used for FormatEntity | |||
| @@ -0,0 +1,40 @@ | |||
| import asteval # used for FormatEntity | |||
| import re # used for FormatEntity | |||
| import asteval # used for FormatEntity | ||
| import re # used for FormatEntity | ||
|
|
||
| from dripline.core import calibrate, ThrowReply |
| ARG img_repo=dripline-python | ||
| #ARG img_tag=develop-dev | ||
| ARG img_tag=receiver-test | ||
| ARG img_tag=v5.0.0-dev |
We created two new endpoint entities
The Entities are used within the MATS setup for some time and seem to work fine.