Adding agent to monitor external fuel tanks level. #639
Conversation
There was a problem hiding this comment.
Thanks for the PR @ntampier! A few comments below. Happy to discuss.
|
|
||
| def __init__(self): | ||
|
|
||
| self.client = serial.Serial("/dev/ttyUSB1", 9600, timeout=0.5) |
There was a problem hiding this comment.
This /dev/ttyUSB1 will need to be configurable, as it is not guaranteed to be the same every time the device is plugged in/the computer is rebooted. I'd recommend passing it as an argument when instantiating a TankLevelMonitor object.
There was a problem hiding this comment.
One overall comment here is that we need a documentation page for the Agent. I described this recently in #638, so I'll link you there: #638 (comment)
|
|
||
| def __init__(self, agent, unit=1, sample_interval=15.): | ||
|
|
||
| self.unit = unit |
There was a problem hiding this comment.
This self.unit is unused. (It looks like it's leftover from the Agent this one was based on.) Remove, along with the unit argument.
| pgroup.add_argument("--unit", default=1, | ||
| help="unit to listen to.") |
There was a problem hiding this comment.
Remove the --unit argument.
| agent, runner = ocs_agent.init_site_agent(args) | ||
|
|
||
| p = TankLevelMonitorAgent(agent, | ||
| unit=int(args.unit), |
There was a problem hiding this comment.
Last place to remove unit.
| def tank_data_checker(self, tank_num): | ||
| self.tank_data_ok = False | ||
|
|
||
| if len(self.full_data) == 89: | ||
| if self.full_data[18:19] != b'': | ||
| if self.verbose: | ||
| print("IN DATA CHECKER 1: ", self.full_data[0:2]) | ||
| if self.full_data[0:2] == b'\x01i': | ||
| if self.verbose: | ||
| print("IN DATA CHECKER 2:", self.full_data[88:89]) | ||
| if self.full_data[88:89] == b'\x03': | ||
| if self.verbose: | ||
| print("IN DATA CHECKER 3:", self.full_data[18:19]) | ||
| if int(self.full_data[18:19]) == int(tank_num): | ||
| if self.verbose: | ||
| print("DATA IS OK") | ||
| self.tank_data_ok = True |
There was a problem hiding this comment.
Let's replace this deep nesting with a series of checks. Starting with the assumption that self.tank_data_ok = False, each check should return if the condition means the data is not ok. If all checks pass, then set self.tank_data_ok = True, like this:
def tank_data_checker(self, tank_num):
self.tank_data_ok = False
if len(self.full_data) != 89:
return
if self.full_data[18:19] == b'':
return
if self.verbose:
print("IN DATA CHECKER 1: ", self.full_data[0:2])
if self.full_data[0:2] != b'\x01i':
return
if self.verbose:
print("IN DATA CHECKER 2:", self.full_data[88:89])
if self.full_data[88:89] != b'\x03':
return
if self.verbose:
print("IN DATA CHECKER 3:", self.full_data[18:19])
if int(self.full_data[18:19]) != int(tank_num):
return
if self.verbose:
print("DATA IS OK")
self.tank_data_ok = True
| while True: | ||
| self.tank_data_verbosity(tank_num, "Entering while loop in tank_data_reader") | ||
| self.tank_data_checker(tank_num) | ||
| if self.tank_data_ok: | ||
| break | ||
| else: | ||
| self.client.flushOutput() | ||
| self.client.write(b'01i201' + tank_num) | ||
| self.full_data = {} | ||
| self.full_data = self.client.read(self.tank_length_to_read) | ||
| self.tank_data_verbosity(tank_num, "After tank_data_ok got False, full data: ") | ||
| self.client.flushOutput() | ||
| self.client.flushInput() | ||
| time.sleep(10) |
There was a problem hiding this comment.
I'm a little concerned about this loop, as it potentially goes on forever if the tank data is always bad. How often is the tank data bad? And when it's bad, do you know why?
I would rather not loop here, do the write/read once, check the data quality, and raise an exception if it fails. The main acq Process is already looping to repeat the read. The exception can be caught there and a warning printed about the data being bad, and you can move on to the next loop in acq, while possibly trying to fix the issue causing the data read to fail.
There was a problem hiding this comment.
The problem is that tank data ib bad very often, so it breaks my code after some minutes, data is bad like once per 5 minutes.
There was a problem hiding this comment.
Gotcha, yeah, I'm not proposing we allow it to break your code, just that it gets caught in the loop at the Agent level so we can tell when it's bad.
| multfactor = [3.785, 3.785, 3.785, 2.54 / 100, 2.54 / 100, 5 / 9, 3.785] # correction factor | ||
| addfactor = [0, 0, 0, 0, 0, -32, 0] | ||
|
|
||
| data = {} |
There was a problem hiding this comment.
Similar to above, this line 94 isn't needed and can be removed.
|
|
||
| self.tank_data_ok = False | ||
| self.tank_length_to_read = 89 | ||
| self.verbose = True |
There was a problem hiding this comment.
A verbosity flag works for debugging, but I'll make the same optional suggestion as I made in #640 -- an alternative approach would be to use debug logging. This would allow you to drop the tracking of verbose/verbosity and instead use:
self.log.debug('debug statement')
The advantage here is that the LOGLEVEL environment variable can be changed without modification of this verbosity constant in the code.
If you want to implement this check out the Logging documentation. (@felipecarrero also just implemented this, you can ask him as well.) I wouldn't let it hold up the PR though.
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: