-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: experimental code to extract energy flow direction from status word and apply to power reading #476
base: master
Are you sure you want to change the base?
Conversation
|
a893a6c
to
1d87caf
Compare
successfully tested with a second meter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-as hinted at in the ticket:
the problem with this code is that the "status" word is not attached to the power reading, but to the energy reading (and i'm not even sure if that's guaranteed?!).
also, it is probably not guaranteed that the reading containing the status word is sent BEFORE the power reading
(currently, if the status word was sent after the power reading, the direction would not be applied.).
is it guaranteed that there is only ONE reading with a status word?
what should the code do if there's more than one status word?
previously, the code that processes the readings did not need to keep any state between the individual readings.
passing the "pointer to bool" into _parse()
was the most straightforward way to pass the now-required state information around, but it is not super elegant, we probably want a cleaner way to handle this.
we could (should?) process the status word and perfom actions based on it in the code that calls _parse()
instead of inside _parse()
,
but at that point applying the sign to the power reading becomes more complex,
because we are dealing with readings wrapped in Reading objects and identified by ReadingIdentifiers at that point.
also, to overcome the issue of the status word not necessarily being sent before the power reading,
we would have to first collect the readings and then iterate over them AGAIN to find and manipulate the power reading.
@@ -97,6 +97,12 @@ MeterSML::MeterSML(std::list<Option> options) | |||
/* using default value if not specified */ | |||
_use_local_time = false; | |||
} | |||
try { | |||
_direction_from_status = optlist.lookup_bool(options, "direction_from_status"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the name of this flag sensible?
it should maybe include "power" too, as we apply it to the power channel.
also it could include "sign"....
"derive_power_sign_from_direction_bit_in_status_field" would be correct, but is a bit excessive.
@@ -244,6 +250,7 @@ ssize_t MeterSML::read(std::vector<Reading> &rds, size_t n) { | |||
sml_file *file; | |||
sml_get_list_response *body; | |||
sml_list *entry; | |||
bool direction_from_status = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable has the same name as the property that stores the setting,
which might be confusing.
@@ -321,9 +328,18 @@ bool MeterSML::_parse(sml_list *entry, Reading *rd) { | |||
rd->value(sml_value_to_double(entry->value) * pow(10, scaler)); | |||
} | |||
|
|||
if (_direction_from_status && entry->status) { | |||
*direction_from_status = (*(entry->status->data.status16) & (1 << 5)) == (1 << 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should properly check status->type
before accessing status->data
.
should replace the magic value "5" with a constant.
ReadingIdentifier *rid(new ObisIdentifier(obis)); | ||
rd->identifier(rid); | ||
|
||
if (_direction_from_status && *direction_from_status && | ||
obis == Obis(1, 0, 15, 7, 0, 0xff)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it OK to hardcode this?
will we ever want to apply the direction to any other reading?
…nd apply to power reading
1d87caf
to
408df91
Compare
fixes #431