Dataclasses - #39
Conversation
only motor_responses are parsed into the nested dataclass still need to parse the other statuses in
bring across changes from comms_qtthread to original comms_thread file
remove now duplicate file
bring across changes from comms_qtthread_2 to original comms_thread file and now use the original file in program
remove now duplicate file
needs to be picked apart later as some code may still be needed (ie. watches)
Code was duplicated during the rebase
JamesOHeaDLS
left a comment
JamesOHeaDLS
left a comment
There was a problem hiding this comment.
@M-Chan - I have made a first pass of the code and picked out some of the key changes that are needed. It is probably easiest to make the changes to the dataclasses first as these will then have an impact elsewhere in the code.
I can then review again once these changes are made.
| try: | ||
| # if isinstance(self.pmac, PPmacSshInterface): | ||
| self.update_identity(status.coordinate_systems[0].identifier_i65) | ||
| self.PpmacGlobalStatusScreen.update_status( |
There was a problem hiding this comment.
Need to be careful here as, looking at the original code, the PowerBrick responses include a $ on the hex value which is stripped off before being passed to update_status. The non-PowerBrick responses don't require this character stripping
| i2t_fault = False | ||
| over_current = False | ||
|
|
||
| for motor_row in status.motors: |
There was a problem hiding this comment.
Suggest renaming motor_row to motor as this better matches your new dataclass approach. The motor_row name comes from the way large rows of motor data were passed in the old status update method
| if isinstance(self.pmac, PPmacSshInterface): | ||
| self.__item(motor_row.number - 1, 1).setText( | ||
| str(motor_row.velocity) | ||
| ) | ||
| else: | ||
| self.__item(motor_row.number - 1, 1).setText( | ||
| str(round(float(motor_row.velocity) * self.servoCycleTime, 1)) | ||
| ) |
There was a problem hiding this comment.
Perform the velocity calculation separately so you call a single setText command
eg
if isinstance(self.pmac, PPmacSshInterface):
velocity = ...
else
velocity = ...
self.__item(motor_row.number - 1, 1).setText(velocity)
This is the approach in the current codebase.
| # if int(value[5]) > 0: | ||
| # over_current = True |
There was a problem hiding this comment.
There are 2 amplifier status checks which are performed for PowerBricks - I2tFaultStatus amd OverCurrent. So having a single amplifier_status field in the dataclass is not enough.
| # number: int | ||
| # running: bool | ||
| # in_position: bool | ||
| identifier_i65: int |
There was a problem hiding this comment.
i65 can move into ControllerStatus as it is a per-controller value
| if time.time() - self.parent.pmac.last_comm_time < 1.0 / self.max_pollrate: | ||
| return | ||
| def generate_cmd(self): | ||
| print("generate_cmd \n") |
There was a problem hiding this comment.
We don't want to lose the pollrate calculation. (It may be refactored later but we should keep hold of it for now)
| return None | ||
|
|
||
| cmd = self.generate_cmd() | ||
| return_thing = self.parsed_poll_response(self.parent.pmac.sendCommand(cmd)) |
There was a problem hiding this comment.
Careful - it is better to handle the success/failure response of sendCommand here rather than pass everything into parsed_poll_response
Also, we need a better variable name than 'return_thing' :)
| self.pmac = Mock() | ||
| self.commsThread = Mock() | ||
| self.commsThread.CSNum = 1 | ||
| self.comms_worker = Mock() |
There was a problem hiding this comment.
This should be reverted again to commsThread as done on the polling branch. Neither will work at the moment in the test, but it will be easier to correct the test as part of your test branch
| QMainWindow.__init__(self, parent) | ||
| self.pmac = Mock() | ||
| self.commsThread = Mock() | ||
| self.comms_worker = Mock() |
There was a problem hiding this comment.
Same as previous comment - revert to commsThread
2 participants
Use dataclasses for polling status