Additional OBIS PF, Average Net Power#47
Conversation
|
Thanks a lot for the pull request. |
|
No, only Excel sheets from energy provider |
8825cbc to
a599bce
Compare
|
Great. Thanks. It is enough. I will add it to the docs. |
36960ba to
7935d49
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for parsing negative numeric values in the DSMR parser and introduces several new OBIS field definitions for Lithuanian smart meters, including instantaneous power factor measurements and average net power demand.
Changes:
- Modified numeric parser to support negative values by changing from uint32_t to int32_t and adding negative sign handling
- Added six new OBIS field definitions: active_demand_net (previously commented out), power_factor, power_factor_l1/l2/l3, and min_power_factor
- Updated test suite to verify parsing of negative values and new dimensionless power factor fields
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/dsmr_parser/parser.h | Modified NumParser to support negative numbers by changing return type to int32_t, adding negative sign detection, and updating value accumulation logic |
| src/dsmr_parser/util.h | Added explicit static_cast in succeed() method to support type conversions when assigning parsed int32_t values to different target types |
| src/dsmr_parser/fields.h | Changed internal value storage from uint32_t to int32_t throughout, enabled active_demand_net field definition, and added five new power factor field definitions using units::none |
| tests/parser_test.cpp | Extended test data with negative power value and dimensionless power factor values, added corresponding field declarations and assertions to verify correct parsing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
10b77db to
af56cb1
Compare
|
Are you ok with the current changes? |
|
I think so ;) |
|
hi @PolarGoose, i checked whole list, i think i still missing one OBIS :)
It's not so big deal, but it could be added to same PR |
|
I have added
This is an incorrect example. I used |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/dsmr_parser/parser.h:160
- The numeric parsing logic does not check for integer overflow when multiplying value by 10. For signed int32_t, values can overflow when parsing large numbers. Consider adding overflow checks before the multiplication operations on lines 139, 152, and 160 to prevent undefined behavior. For example, check if value would exceed INT32_MAX/10 before multiplying.
value *= 10;
value += (*cur_symbol - '0');
++cur_symbol;
}
// Parse decimal part, if any
if (max_decimals && cur_symbol < end && *cur_symbol == '.') {
++cur_symbol;
while (cur_symbol < end && !strchr("*)", *cur_symbol) && max_decimals) {
max_decimals--;
if (*cur_symbol < '0' || *cur_symbol > '9')
return res.fail(INVALID_NUMBER, cur_symbol);
value *= 10;
value += (*cur_symbol - '0');
++cur_symbol;
}
}
// Fill in missing decimals with zeroes
while (max_decimals--)
value *= 10;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Add negative value support
I have added additional obis which was missing for Lithuania meters
1-0:1.24.0 also can have negative value (generation/export)