numat / alicat

Python driver and command line tool for Alicat mass flow controllers.
GNU General Public License v2.0
21 stars 27 forks source link

CLI not using await on set_flow_rate() #25

Closed alexrudd2 closed 2 years ago

alexrudd2 commented 2 years ago
a.ruddick@Alexs-laptop alicat % alicat tcp://192.168.1.212:4000 --set-flow-rate 1
/Users/a.ruddick/Documents/github/alicat/alicat/tcp.py:310: RuntimeWarning: coroutine 'FlowController.set_flow_rate' was never awaited
  flow_controller.set_flow_rate(args.set_flow_rate)
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
{
  "control_point": "flow",
  "gas": "N2",
  "mass_flow": -0.0,
  "pressure": 14.29,
  "setpoint": 0.0,
  "temperature": 21.66,
  "volumetric_flow": 0.0
}

https://github.com/numat/alicat/blob/master/alicat/tcp.py#L309-L312

It looks like the lack of await causes some sort of race condition where the flow doesn't actually get set. Compare to the below version (with await added in)

a.ruddick@Alexs-laptop alicat % alicat tcp://192.168.1.212:4000 --set-flow-rate 1
{
  "control_point": "flow",
  "gas": "N2",
  "mass_flow": 0.0,
  "pressure": 14.3,
  "setpoint": 1.0,
  "temperature": 21.68,
  "volumetric_flow": 0.0
}
alexrudd2 commented 2 years ago

Additionally, even after adding await there's another problem.

The method set_flow_rate() (same as set_pressure()) has a check to see if self.control_point is set to either flow or pressure before sending a new setpoint. Since it's initially not known, FlowController.__init__() schedules the coroutine _get_control_point. However, since it's using asyncio.ensure_future instead of await the task doesn't actually finish and update self.control_point before set_flow_rate() starts.

The good news is, the use of a lock means the two functions don't receive data intended for the other. The bad news is, since self.control_point is unknown at this point, set_flow_rate() will always try to set the flow to 0 and change the control point, even if the Alicat actually is in flow. This needlessly causes a bump.

a.ruddick@Alexs-laptop alicat % alicat tcp://192.168.1.212:4000 --set-flow 30
13:26:45.597591 trying lock for command AR122       //_get_control_point from __init__
13:26:45.597646 got lock for command AR122
13:26:45.597790 trying lock for command AS0.00.    //set_flow_rate() first closes the Alicat because
13:26:45.625818 reply A   122 = 34              //the control point isn't known until now
13:26:45.625974 got lock for command AS0.00
13:26:45.695220 reply A +029.52 +020.04 +0102.6 +0209.5 000.00      N2
13:26:45.695427 trying lock for command AW122=34 //set_flow_rate() needlessly tries to change flow-> flow
13:26:45.695469 got lock for command AW122=34
13:26:45.762996 reply A   122 = 34
13:26:45.763177 trying lock for command AS30.00           //finally the command we wanted
13:26:45.763217 got lock for command AS30.00
13:26:45.853776 reply A +021.10 +020.04 +0000.4 +0000.1 030.00      N2 
13:26:45.854028 trying lock for command A
13:26:45.854063 got lock for command A
13:26:45.910137 reply A +021.24 +020.04 +0001.2 +0001.5 030.00      N2
alexrudd2 commented 2 years ago

@patrickfuller Thanks to our discussion yesterday and a rethink today, a clean answer became apparent. asyncio.Lock() to the rescue again. It can be defined in the sync constructor, and acquired by both coroutines.

a.ruddick@Alexs-laptop alicat % alicat tcp://192.168.1.212:4004  --set-pressure 0
12:17:58.644522 trying lock for command AR122
12:17:58.644552 got lock for command AR122
12:17:58.708222 reply A   122 = 34
12:17:58.708305 trying lock for command AS0.00 //with the second (unlogged) lock, set_flow_rate() waited
12:17:58.708319 got lock for command AS0.00
12:17:58.789095 reply A +014.53 +022.32 -0002.3 -0002.3 000.00      N2
12:17:58.789170 trying lock for command A
12:17:58.789187 got lock for command A
12:17:58.859386 reply A +014.53 +022.32 -0002.3 -0002.3 000.00      N2