Skip to content
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

Add myuplink specific sensor descriptions #109867

Merged

Conversation

astrandb
Copy link
Contributor

@astrandb astrandb commented Feb 7, 2024

Breaking change

Proposed change

Add support for more sensor descriptions to enable assignment of device_classes and more. These descriptions can be global for each parameter_id or specific for a certain categgory (i.e. heat pump model).

If the device_class is set to ENUM the options will be extracted from enumValues.

Needs myuplink lib >= 0.1.0

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

home-assistant bot commented Feb 7, 2024

Hey there @pajzo, mind taking a look at this pull request as it has been labeled with an integration (myuplink) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of myuplink can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign myuplink Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@astrandb
Copy link
Contributor Author

astrandb commented Feb 7, 2024

Thank you @MartinHjelmare for your valuable comments and suggestions. Much cleaner now.
I will keep the PR as draft until the lib dependency bump is merged.

homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
@astrandb astrandb marked this pull request as ready for review February 7, 2024 18:16
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
key="priority",
device_class=SensorDeviceClass.ENUM,
icon="mdi:priority-high",
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S series an F series have overlapping number series (at least with the old f series numbering) so this might not be enough. I kind of doubt that myuplink upgraded F series have adapted the numbering of the S series.

Ive not looked at the numbering for S series on myuplink uet but it would sort of make more sense if matched modbus numbers, but maybe it doesnt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are prepared for overlapping numbers. The numbers in the descriptor keys can be qualified with category name if needed. However I suspect that this can be a source of more work until everything calmes down...
I would really like to know how S-series presents itself in the myuplink API to try to find the most efficient solution.
The new firmware for my F730 was released about two weeks ago and i upgraded and migrated last week and quite a lot of sensors are missing in the public API. I noted that Nibe/myUplink have made some significant upgrades to the capabilities in the ios-app and on the web for F730 the last few days.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far as S series and F series never have same number for anything. So i think it should always be qualified.

If want to see the mostly complete databases, they are here https://github.com/yozik04/nibe/tree/master/nibe/data

We can probably ask some S series guys using the modbus setup test the myuplink integration of you dont have any such testers. Once the new release is public i suspect they will test it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elupus when you mention S serie, i have a NIBE VVM S320 E EM 3x400V DK - what do you want me to test?
Without fully understand you point, i think it's fine to have these parameter_id config, and fallback to parameter_unit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pajzo what does your parameter 43108 represent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or 49994? Or some other parameter for 'priority' you have

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pajzo what does your parameter 43108 represent

It is "Current fan mode" in the F730, = [0, 1, 2, 3, 4] It is assigned % as unit which is incorrect.

49994 is labelled Priority and can be Off, Hot water, Heating, Pool and so on.

From the metadata in each device-point I think we can make an educated guess what entity platform that is suitable. E.g. a parameter that can be zero or one and is writable should be represented by a switch. If it is read-only, a binary sensor would be appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im asking for S series report from @pajzo . As i said their number series are very likely overlapping and with completely different meaning for same number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I fully understand that there are overlapping number series.
Please regard the current code more of a proof-of-concept than the ultimate solution. If we change the "49994" key to "NIBEF F730 CU 3x400V-49994" it will be correct for F730 and the sensor will show a number value with default icon for all other devices with a "49994".
I am currently working on a better method to interpret the point metadata and assign proper platform, device_class and other properties. Hopefully this will be reasonably correct. The ambition should be that the need for "manual" overrides is not too big.
The overall aim with the intergration is to make use of the metadata and do the best we can with it. The alternative is to maintain a huge database from all manufacturers connected to myUplink and all their models and that seems rather unattractive to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a output for my S320 - seems like the parameter_id's is unique.

[
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "4",
        "parameterName": "Current outdoor temper­ature (BT1)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:53:53+00:00",
        "value": 0.4,
        "strVal": "0.4°C",
        "smartHomeCategories": [
            "sh-outdoorTemp"
        ],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "8",
        "parameterName": "Supply line (BT2)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:54:54+00:00",
        "value": 38.4,
        "strVal": "38.4°C",
        "smartHomeCategories": [
            "sh-supplyTemp"
        ],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "10",
        "parameterName": "Return line (BT3)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:54:38+00:00",
        "value": 28.2,
        "strVal": "28.2°C",
        "smartHomeCategories": [
            "sh-returnTemp"
        ],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "11",
        "parameterName": "Hot water top (BT7)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:46:44+00:00",
        "value": 48.8,
        "strVal": "48.8°C",
        "smartHomeCategories": [
            "sh-hwTemp"
        ],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "12",
        "parameterName": "Hot water char­ging (BT6)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:47:25+00:00",
        "value": 46.0,
        "strVal": "46°C",
        "smartHomeCategories": [
            "sh-hwTemp"
        ],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "54",
        "parameterName": "Average temper­ature (BT1)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-08T22:58:57+00:00",
        "value": -0.6,
        "strVal": "-0.6°C",
        "smartHomeCategories": [
            "sh-outdoorTemp"
        ],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "781",
        "parameterName": "Degree minutes",
        "parameterUnit": "DM",
        "writable": true,
        "timestamp": "2024-02-09T03:54:58+00:00",
        "value": -796.0,
        "strVal": "-796DM",
        "smartHomeCategories": [],
        "minValue": -30000.0,
        "maxValue": 30000.0,
        "stepValue": 20.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "1708",
        "parameterName": "Calcu­lated supply climate system 1",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:40:12+00:00",
        "value": 35.0,
        "strVal": "35°C",
        "smartHomeCategories": [
            "sh-supplyTemp"
        ],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "Varmepumpe 1",
        "parameterId": "2491",
        "parameterName": "Return line (EB101-BT3)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:55:09+00:00",
        "value": 27.9,
        "strVal": "27.9°C",
        "smartHomeCategories": [],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "Varmepumpe 1",
        "parameterId": "2494",
        "parameterName": "Supply line (EB101-BT12)",
        "parameterUnit": "°C",
        "writable": false,
        "timestamp": "2024-02-09T03:53:54+00:00",
        "value": 36.8,
        "strVal": "36.8°C",
        "smartHomeCategories": [],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [],
        "scaleValue": "0.1",
        "zoneId": null
    },
    {
        "category": "NIBE VVM S320 E EM 3x400V DK",
        "parameterId": "10895",
        "parameterName": "Pump: Heating medium (GP6)",
        "parameterUnit": "",
        "writable": false,
        "timestamp": "2024-02-08T23:45:49+00:00",
        "value": 0.0,
        "strVal": "0",
        "smartHomeCategories": [],
        "minValue": null,
        "maxValue": null,
        "stepValue": 1.0,
        "enumValues": [
            {
                "value": "0",
                "text": "0",
                "icon": ""
            },
            {
                "value": "1",
                "text": "1",
                "icon": ""
            }
        ],
        "scaleValue": "1",
        "zoneId": null
    }
]

@MartinHjelmare MartinHjelmare changed the title Add specific sensor descriptions Add myuplink specific sensor descriptions Feb 8, 2024
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Are we ready to merge?

Copy link
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. These numbers are not a unique identifier for a parameter. Same number can mean different things on different series. It need to be series qualified.

@home-assistant home-assistant bot marked this pull request as draft February 8, 2024 18:24
@home-assistant
Copy link

home-assistant bot commented Feb 8, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Copy link
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is too strict qualification. We cant duplicate this for all pump types. All Fxxxx series pumps (from nibe) have the same series of numbers.

@astrandb
Copy link
Contributor Author

astrandb commented Feb 8, 2024

That is too strict qualification. We cant duplicate this for all pump types.

Yes, I agree. But this is not the final version. It will be OK for F730 and all other the models will get same generic sensors for all unspecific parameters as before this PR.

So I think we can merge this to be able to proceed.

Then we need diagnostic data from a variety of pumps in "myUplink terminology" to be able to figure out a method for grouping based on available metadata.

I am almost ready with a solution that sorts out which DevicePoints that should create sensors, sensors with enum device_class, binary sensors and switches repectively. All by looking at the metadata from the API. Those features will come in later PR:s.

However I hope the diagnostics.py can be merged soon. That is a simple way to get more information from the early adopters of the integration.

@astrandb astrandb marked this pull request as ready for review February 8, 2024 19:14
@astrandb
Copy link
Contributor Author

astrandb commented Feb 8, 2024

All Fxxxx series pumps (from nibe) have the same series of numbers.

OK thats good to know. Then it simple to qualify with "NIBEF F" from the category string. But it would be nice to have seen real data from one or two other models instead of guessing how the API categorizes the models.

I can produce follow up PR with this more liberal model for qualification - it will probably not harm.

@elupus
Copy link
Contributor

elupus commented Feb 8, 2024

All Fxxxx series pumps (from nibe) have the same series of numbers.

OK thats good to know. Then it simple to qualify with "NIBEF F" from the category string. But it would be nice to have seen real data from one or two other models instead of guessing how the API categorizes the models.

I can produce follow up PR with this more liberal model for qualification - it will probably not harm.

My guess would be "NIBEF" and "NIBEF" prefix on the category. The second F in "NIBEF FXXX" is part of the model name.

So if you link it to to "NIBEF" for now, that is okey.

@astrandb
Copy link
Contributor Author

astrandb commented Feb 8, 2024

Ready for final review. ?!

@@ -102,4 +157,36 @@ def __init__(
def native_value(self) -> StateType:
"""Sensor state value."""
device_point = self.coordinator.data.points[self.device_id][self.point_id]
if device_point.value == -32768:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this check mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally the API will not include device_point that are permanently unavailable. The API will return -32768 if data is temporarily unavailable. I.e. "Unknown" with HA terminolgy. It happens on two data_points in my F730. I think it is better to show "Unknown" than showing -32768.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That constant kind of needs to be defined in the library, but it at least need a constant defined for it here.

@astrandb
Copy link
Contributor Author

astrandb commented Feb 9, 2024

Example from real life

image

@MartinHjelmare MartinHjelmare marked this pull request as draft February 12, 2024 11:33
@astrandb astrandb marked this pull request as ready for review February 12, 2024 12:40
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft February 12, 2024 13:37
@astrandb astrandb marked this pull request as ready for review February 12, 2024 13:55
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@elupus are you ok with this?

@astrandb
Copy link
Contributor Author

Screenshot from heat-pump in Orsa

image

Copy link
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should be okey. Its early yet so stuff can ve refactored if needed later. One small question, but can also be looked at later.

homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/myuplink/sensor.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare merged commit 29ed823 into home-assistant:dev Feb 12, 2024
23 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants