-
Notifications
You must be signed in to change notification settings - Fork 537
Edge2Edge: filter undefined integer modbus values #3479
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3479 +/- ##
=============================================
+ Coverage 59.61% 59.62% +0.01%
Complexity 105 105
=============================================
Files 3004 3004
Lines 130410 130427 +17
Branches 9637 9639 +2
=============================================
+ Hits 77737 77754 +17
+ Misses 49764 49762 -2
- Partials 2909 2911 +2 🚀 New features to boost your workflow:
|
36b15ca to
df8f641
Compare
tsicking
left a comment
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.
Thanks for the helpful PR. I have some comments, mostly of code style nature.
io.openems.edge.edge2edge/src/io/openems/edge/edge2edge/common/AbstractEdge2Edge.java
Outdated
Show resolved
Hide resolved
io.openems.edge.edge2edge/src/io/openems/edge/edge2edge/common/AbstractEdge2Edge.java
Outdated
Show resolved
Hide resolved
io.openems.edge.edge2edge/src/io/openems/edge/edge2edge/common/AbstractEdge2Edge.java
Outdated
Show resolved
Hide resolved
io.openems.edge.edge2edge/src/io/openems/edge/edge2edge/common/AbstractEdge2Edge.java
Outdated
Show resolved
Hide resolved
| */ | ||
| private ElementToChannelConverter getConverterForType(ModbusType type) { | ||
| return switch (type) { | ||
| case UINT16 -> ElementToChannelConverter.SET_NULL_FOR_DEFAULT(0xFFFF); // 65535 |
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.
I wonder if there is any way to use ModbusRecordUint16.UNDEFINED_VALUE instead of 0xFFFF as I suggested in the forum, but it's not too important.
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.
thanks for your feedback! I tried to do that but I couldn't find a nice way of getting the byte array ModbusRecordUint16.UNDEFINED_VALUE to work with this code.
Also I ran into problems with casting between unsigned types Uint16-64 and signed Java types Short, Int, Long. Having it as 0xFFFF worked.
|
Is it feasible to add some kind of unit test for this? |
- Add UNDEFINED_VALUE and UNDEFINED_BYTE_ARRAY - Add JUnit tests
|
I used a different approach, reusing the existing UNDEFINED_VALUEs in 41db621. I added JUnit tests, but did not test it in production yet. Could anybody test it? |
This change fixes an issue with the internal OpenEMS Edge-2-Edge communication. It has been discussed in this forum post.
Short Summary: when an OpenEMS edge has an
undefinedinteger channel value, it sets the value to the max. int value (e.g. 65535 for 16bit unsigned integer) in the Modbus API. The master edge then reads this Modbus value, but doesn't recognize it asundefined. Instead, the value gets used just like any other value. This can lead to problems like SOC of 65535%.This fix adds a converter for integer values to the
AbstractEdge2Edgeclass, that sets channel values tonull, if the Modbus value isundefined.