Supply allowed string values of @AttributeDefinition channels

[Lamarqe]

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"
        ]
    },

[Sn0w3y]

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

?

[Lamarqe]

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

[Lamarqe]

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

[codecov]

Codecov Report

❌ Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #3020      +/-   ##
=============================================
+ Coverage      55.64%   55.68%   +0.05%     
- Complexity     19016    19025       +9     
=============================================
  Files           3670     3670              
  Lines         171447   171480      +33     
  Branches       14113    14119       +6     
=============================================
+ Hits           95377    95475      +98     
+ Misses         72302    72232      -70     
- Partials        3768     3773       +5     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sfeilmeier]

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)

[Lamarqe]

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]

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

[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.

[Sn0w3y]

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! :)

[Lamarqe]

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.

[sfeilmeier]

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 Fix handling of URLs with square brackets in Jetty Rest API #3068)

[github-actions]

This PR has been automatically marked as stale due to inactivity

If it is still relevant, please update the PR.
Otherwise, it will be automatically closed in 14 days.

Before updating, please review our Contribution Guidelines to ensure all required information is provided.

[github-actions]

This PR has been automatically marked as stale due to inactivity

If it is still relevant, please update the PR.
Otherwise, it will be automatically closed in 14 days.

Before updating, please review our Contribution Guidelines to ensure all required information is provided.

[Lamarqe]

Hi @sfeilmeier,

As this would be a really valuable addition, I went over it again with the intention to adjust the implementation according to your feedback and create unit test coverage. Regarding your implementation comment:

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)

This is exactly what my implementation does. The EdgeConfig enum parsing stores the options in the schema field templateOptions->options. My implementation gets this information exactly in the point you recommend ("where the Channel is created") See (https://github.com/Lamarqe/openems/blob/c28f59b9c4c127731a7f05f459cef21a6466a7d9/io.openems.edge.common/src/io/openems/edge/common/component/AbstractOpenemsComponent.java#L359-L364)

If this is not adequate: Could you give a few more details of how you would like to have the implementation changed?

[Lamarqe]

As part of re-basing, I noticed a recent code change which is similar to the extension made with my PR. This gives me better confidence that my changes are aligned with the existing implementation.

Also, I added today the unit tests requested by Stefan.
With that, I am sending it again for review with the request to merge.

[Lamarqe]

@sfeilmeier : Can we please move on with this PR? Earlier review comments have been incorporated / answered.