Feat/fix alicat multidrop - #2
Conversation
Removed unit from display until dynamic units are implemented
| { | ||
| private readonly List<char> _unusedIds; | ||
| //simple registry to manage instances of seriaport resources across multiple mfcs | ||
| static Dictionary<string, MassFlowControllerConnection> _connections = new Dictionary<string, MassFlowControllerConnection>(); |
There was a problem hiding this comment.
I'm curious if you could elaborate on what you wanted to implement this to address. Typically we don't expect the driver to care about it's connection in relation to any others (maintaining the idea that the driver itself would only really care about it's own connection). It's true that the MFC's can share a serial connection, and we account for that in ARES by allowing devices of the same class (i.e. Alicat MFC's) to share the same resource with a unique identifier. For the MFC's this is the Id list that use a single capital letter (A, B, C...). Unless there's something critical this provides, I would say this is best left at the ARES level rather than the driver level itself.
| _serialConnection = new MassFlowControllerConnection(serialInfo.PortName); | ||
| _serialConnection = MassFlowControllerConnection.GetMassFlowControllerConnection(serialInfo.PortName); | ||
|
|
||
| _stateWatchers = new CompositeDisposable |
There was a problem hiding this comment.
I see the motivation here, but this should theoretically not be an issue on the current system. When receiving data that needs to be parsed, we have dedicated response parser classes that handle breaking down the serialized data. In the case of this stream, we're only going to get this processed response (the "LiveDataResponse" class) in the event that parser deemed it a valid response to parse. You can find that logic under "Commands/Responses/Parsers/LiveDataParser.cs". Based on that, I don't think this change is necessary, you shouldn't see the system processing any additional data from MFC's other than the one you told the driver to care about. If you are, then perhaps there's another bug happening under the hood we can dig out.
| private void UpdateLiveData(LiveDataResponse liveResponse) | ||
| { | ||
| _liveData = liveResponse; | ||
| /// check on id is required if _statewatcher is used in order to avoid updating the state with a response from a different MFC than the one that is being watched |
There was a problem hiding this comment.
My comment above talks about this point I believe
| var flowVal = StandardVolumeFlow.From(numericNum, unit); | ||
| // must be converted to match setpoint units, otherwise may cause issues when calculating newsetpoint | ||
| // dataFrameFormat.MaxVal = flowVal.StandardLitersPerMinute.ToString(); | ||
| dataFrameFormat.MaxVal = flowVal.As((StandardVolumeFlowUnit)dataFrameFormat.Unit).ToString(); |
There was a problem hiding this comment.
Just curious, was this change inspired by an issue you saw in lab?
3 participants
Alicat devices are designed to have multiple devices sharing a single serial port. This fix implements a simple registry to allow multiple devices to share the same connection. Also updated the device so that livedata update is treated as a query instead of listening to a transaction stream. Included notes on implementation if use of transaction stream needed to be restored.