openems icon indicating copy to clipboard operation
openems copied to clipboard

Supply allowed string values of @AttributeDefinition channels

Open Lamarqe opened this issue 10 months ago • 12 comments

Until now, AttributeDefinition channels (channels, whose names start with _Property) which are based on Enums do not provide the allowed values in their type defintions retrieved via edge call getChannelsOfComponent().

For example the channel ctrlEvcs0/_PropertyChargeMode provides:

    {
        "id": "_PropertyChargeMode",
        "accessMode": "RO",
        "persistencePriority": "HIGH",
        "text": "",
        "type": "STRING",
        "unit": "",
        "category": "OPENEMS_TYPE"
    },

With this pull request, the allowed enum values (which were stored internally already previously) are published correctly:

    {
        "id": "_PropertyChargeMode",
        "accessMode": "RO",
        "persistencePriority": "HIGH",
        "text": "",
        "type": "STRING",
        "unit": "",
        "category": "OPENEMS_TYPE",
        "options": [
            "FORCE_CHARGE",
            "EXCESS_POWER"
        ]
    },

Lamarqe avatar Feb 20 '25 20:02 Lamarqe

Does this solve the Issue

Feb 20 21:56:23 server.io java[1239557]: 2025-02-20T21:56:23,788 [socket-1] WARN [nflux.FieldTypeConflictHandler] [Timedata.InfluxDB] Unable to convert field [_appManager/_PropertyApps] value ["[\n {\n \"appId\": \"App.Api.ModbusTcp.ReadOnly\",\n \"alias\": \"\",\n \"instanceId\": \"876b92bf-0fa4-4dc6-8cfc-cb0d49d3db70\",\n \"properties\": {\n \"CONTROLLER_ID\": \"ctrlApiModbusTcp0\"\n }\n },\n {\n \"appId\": \"App.Api.RestJson.ReadOnly\",\n \"alias\": \"\",\n \"instanceId\": \"5c989095-7496-4ec4-b142-7aafbf03b37c\",\n \"properties\": {\n \"CONTROLLER_ID\": \"ctrlApiRest0\"\n }\n }\n]"] to integer Feb 20 21:56:23 server.io java[1239557]: 2025-02-20T21:56:23,788 [socket-1] WARN [nflux.FieldTypeConflictHandler] [Timedata.InfluxDB] Unable to convert field [_appManager/_PropertyCurrency] value ["EUR"] to integer Feb 20 21:56:23 server.io java[1239557]: 2025-02-20T21:56:23,788 [socket-1] WARN [nflux.FieldTypeConflictHandler] [Timedata.InfluxDB] Unable to convert field [_appManager/_PropertyIgnoreStateComponents] value ["[\"\"]"] to integer Feb 20 21:56:23 server.io java[1239557]: 2025-02-20T21:56:23,788 [socket-1] WARN [nflux.FieldTypeConflictHandler] [Timedata.InfluxDB] Unable to convert field [_appManager/_PropertyKeyForFreeApps] value ["0000-0000-0000-0000"] to integer

?

Sn0w3y avatar Feb 20 '25 20:02 Sn0w3y

No idea. I don't see any connection to this pull request.

Lamarqe avatar Feb 20 '25 21:02 Lamarqe

Is there anything which I need to do in order to receive a review or other feedback about how to proceed?

Lamarqe avatar Feb 26 '25 22:02 Lamarqe

Codecov Report

:x: Patch coverage is 0% with 38 lines in your changes missing coverage. Please review.

:x: Your patch check has failed because the patch coverage (0.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #3020      +/-   ##
=============================================
+ Coverage      59.51%   59.51%   +0.01%     
  Complexity       113      113              
=============================================
  Files           2765     2765              
  Lines         119513   119546      +33     
  Branches        8892     8898       +6     
=============================================
+ Hits           71114    71137      +23     
- Misses         45723    45733      +10     
  Partials        2676     2676              
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 28 '25 14:02 codecov[bot]

Thank you for the contribution. I'll need some time to review it in detail. Ideally I would like to not add it to the parent Doc directly as it relates only to Enums. Also the code does not fully align with our coding guidlines (https://openems.github.io/openems.io/openems/latest/contribute/coding-guidelines.html)

sfeilmeier avatar Mar 01 '25 15:03 sfeilmeier

Thank you for having a first look and taking the time for a full review. I guess with "parent doc", you refer to Doc.java? I did not find a way to modify only AbstractDoc.java. Logically, its an enum channel, but technically, it is not. (Its ChannelCategory.OPENEMS_TYPE using OpenemsType.STRING. The only difference is the schema it comes with.)

Regarding the the style warnings: During the last hour, I applied the Eclipse code formatter to all modified files and updated the branch accordingly. PR should be clean now.

Lamarqe avatar Mar 09 '25 21:03 Lamarqe

@sfeilmeier : do you think you will find the time to do a full review anytime soon?

Lamarqe avatar Mar 20 '25 23:03 Lamarqe

Is there any chance to get this MR merged? If not, please let me know and share a short explanation

Then I would just close it and stop investing time in doing regularly rebases and retests.

Lamarqe avatar Apr 04 '25 20:04 Lamarqe

Is there any chance to get this MR merged? If not, please let me know and share a short explanation

Then I would just close it and stop investing time in doing regularly rebases and retests.

If i am honest i do not know the exact Usecase as neither the UI nor any other Component uses them incorrectly - if you would provide a little more Use-Case info it would be good i guess. I am not in the Position to Merge nor to reject - just my Questions! :)

Sn0w3y avatar Apr 04 '25 20:04 Sn0w3y

Yes, valid and good question.

Having this information available in the channel definition will allow for a generic integration into other smart home systems. Instead of hard-coding behaviors and options, integrations to connected systems can use the information retrieved via the GetChannelsOfComponent reponse to define possible user and system interactions.

One example of such a connected system could be Home Assistant. But this MR is fully independent of the concrete connected system.

Lamarqe avatar Apr 05 '25 16:04 Lamarqe

Sorry, I hardly find time to work on this feature. The two reasons why I don't immediately accept your current implementation:

  • In EdgeConfig we already parse Enums correctly, when creating the Schema (https://github.com/OpenEMS/openems/blob/develop/io.openems.common/src/io/openems/common/types/EdgeConfig.java#L821). This info would have to be propagated to where the Channel is created (https://github.com/OpenEMS/openems/blob/develop/io.openems.edge.common/src/io/openems/edge/common/component/AbstractOpenemsComponent.java#L306)
  • There are no JUnit tests to validate this new behaviour. We just had a similar issue with our API for external home automation systems which also only happend because nobody was aware a dependency update could break it (see https://github.com/OpenEMS/openems/pull/3068)

sfeilmeier avatar Apr 13 '25 20:04 sfeilmeier