-
Notifications
You must be signed in to change notification settings - Fork 400
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
SunSpecCodeGenerator: refactor, add 7xx models, use JSON input files #2324
SunSpecCodeGenerator: refactor, add 7xx models, use JSON input files #2324
Conversation
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.
Great replacement of the existing code.
Like that you sorted the json files before generating the output code. This makes the generated code more easily comparable.
Still a bit unsure about missing S303. Could imagine that some Inverters use it to transmit the temperature. But as no one is using it, it is probably okay to miss it.
@sfeilmeier As Sunspec is core functionality, could someone from your team have a look at it. |
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.
Thank you for your contribution!
public static String getAsStringOrElse(JsonElement jElement, String memberName, String alternative) { | ||
try { | ||
return getAsString(jElement, memberName); | ||
} catch (OpenemsNamedException e) { |
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.
Exceptions should not be used in normal control flow (e.g. https://dzone.com/articles/exceptions-as-controlflow-in-java)
I am replacing it with return getAsOptionalString(jElement, memberName).orElse(alternative);
Also JUnit test was missing.
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 a lot, I wasn't aware of that. I'll keep it in mind.
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.
You're welcome. Usually this is not a big issue, but we found this to be a problem when we analyzed performance issues in the Backend Edge.Websocket using Visual.VM (https://visualvm.github.io/). During peak situations (e.g. restart of server) we had thousands of Exceptions per second which caused significant slow down.
- Modbus Bridge: optimize execution of Tasks (#1976) - split TimestampedDatanotification into AggregatedDataNotification, TimestampedDataNotification and ResendDataNotification. Added/enhanced some utilities classes. (#2297) - Modbus Bridge: refactor & cleanup Elements (#2273) - Bump @types/uuid from 9.0.1 to 9.0.2 in /ui (#2225) - Bump com.squareup.okio:okio-jvm from 3.4.0 to 3.5.0 in /cnf (#2307) - Bump org.checkerframework:checker-qual from 3.36.0 to 3.37.0 in /cnf (#2308) - Backend Timedata: prepare new data notification types and utilities (#2298) - Docs: Fix typing error in backend-to-backend section (#2315) - Add Backend Timedata AggregatedInflux (#2313) - Bump info.faljse:SDNotify from 1.3 to 1.5 in /cnf (#2328) - Bump org.rrd4j:rrd4j from 3.8.2 to 3.9 in /cnf (#2327) - Bump org.apache.felix:org.apache.felix.http.jetty from 5.0.4 to 5.0.6 in /cnf (#2326) - ModbusBridge: Fix ClassCastException to Register[] (#2320) - Update Gradle to 8.3 (#2330) - Bump compare-versions from 6.0.0 to 6.1.0 in /ui (#2319) - SunSpecCodeGenerator: refactor, add 7xx models, use JSON input files (#2324) - Docs: Update Getting Started & Implementing a Device (#2331) - CI: build artifacts on tag/release event (#2332) - Bump org.apache.felix:org.apache.felix.http.jetty from 5.0.6 to 5.1.0 in /cnf (#2334) - Bump org.jetbrains.kotlin:kotlin-osgi-bundle from 1.9.0 to 1.9.10 in /cnf (#2335) - Bump io.reactivex.rxjava3:rxjava from 3.1.6 to 3.1.7 in /cnf (#2333)
Based on PR 2304 by nlamarti, this new PR handles the comments made there. Comparing the old and new output file
DefaultSunSpecModel.java
, the following changed:Note that the length of model 701 is 155>126=maximum length of a modbus task in j2mod. As the
AbstractOpenemsSunSpecComponent
adds one task for each block, this causes an error (see issue 2311). I have fixed this already, this will be done in a seperate PR.Furthermore, I used modern syntax in the switch-statements in
SunSpecPoint.java
, as I had to update them anyway due to the new pointn type STRING32.Lastly, I had to write a new method
getAsStringOrElse
inJsonUtils.java
, and the other changes in this file are caused by the automatic code clean up.