Report per phase measurements for correct phase#154
Conversation
|
After a discussion, we'll rewrite this to return the map[ElectricalConnectionPhaseNameType]float64 to be more explicit about whether phases were written to or not. |
|
Another and easier to use option might be a |
f35aaa1 to
6eb1154
Compare
|
I've updated this PR to return a map[ElectricalConnectionPhaseNameType]float64. |
|
We should update other usecases with the new approach to passing/receiving phase-specific values. That task is tracked in #197 |
DerAndereAndi
left a comment
DerAndereAndi
left a comment
There was a problem hiding this comment.
EVCEM is not updated but requires the same changes. Would be great if it would be part of this PR as well
| phaseName = *param[0].AcMeasuredPhases | ||
| } else if validPhaseNameTypes == nil { | ||
| // if we're not filtering by valid phase names, allow acMeasuredPhases to be unset | ||
| phaseName = model.ElectricalConnectionPhaseNameTypeNone |
There was a problem hiding this comment.
Do we have a test for this code path?
| return data[0], nil | ||
| for _, k := range data { | ||
| // If the Monitored Unit is connected to less than three phases, one of the other combinations like "a" or "ab" are allowed instead of "abc". | ||
| // The values "a", "b", and "c" are permitted if and only if only one |
There was a problem hiding this comment.
the comment looks incomplete "... and only if only one ..." what? double only? :)
| // if we're not filtering by valid phase names, allow acMeasuredPhases to be unset | ||
| phaseName = model.ElectricalConnectionPhaseNameTypeNone | ||
| } else { | ||
| // error getting parameter description |
There was a problem hiding this comment.
I don't think this comment is correct here
33f8401 to
dc0455b
Compare
- changes return type of MeasurementPhaseSpecificDataForFilter to a map from PhaseName to value instead of a list
…set validPhaseNameTypes
…lter by computing key from AcMeasuredPhases AND AcMeasuredInReferenceTo (fixes phase to phase voltages overwriting phase to neutral voltages)
dc0455b to
6e53f8b
Compare
4 participants
If/when a device registers PhaseSpecific measurements only for one specific phase (or registers them for all phases but only provides measurements for one of them), MeasurementPhaseSpecificDataForFilter() returns a []float64 containing only 1 value with no way for the API user to know for which phase the measurement was meant. This is an issue in e.g. ma/mpc's PowerPerPhase which would then return an array with less than 3 members and callers would then not know to which phase the measurement corresponds.
This PR changes the return type of MeasurementPhaseSpecificDataForFilter to always contain one entry per phase so that for a device which only reports data on phase B, it would return []float64{0, 10, 0} instead of []float64{10}. The advantage of this approach is that it doesn't change the return type of MeasurementPhaseSpecificDataForFilter or any of its callers, the downside is that callers can't (easily) distinguish devices that are reporting 0 for a phase and devices that aren't reporting a phase at all. The alternative would be to change the return type from []float64 to map[ElectricalConnectionPhaseNameType]float64 and then going through all the callers to make them use/return that as well.
I'm not particular to either approach so I picked the one that changed the surface API less.