-
-
Notifications
You must be signed in to change notification settings - Fork 36.3k
Add support for raw sensor data to take different data types #145070
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 support for raw sensor data to take different data types #145070
Conversation
|
Hey there @tuya, @zlinoliver, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
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.
Hi @FredrikM97, and fix change type selection 😋
@davidrapan What do you mean? |
|
Updated to be better backwards compatible (default to ElectricityTypeData) and also fixed the type for the native_value. |
|
I would suggest as a preliminary PR to add a fixture files with your pet feeder details.
(see #148401 as a sample) |
@epenet I could split the sensor code (meal_plan) to another PR instead. The main focus in this PR is to solve the RAW base64 and not force it to be a ElectricityTypeData. |
For sure everything that can be done in a separate PR is best |
|
@epenet I've now separated it so that this PR focuses solely on allowing RAW data to be types other than ElectricityTypeData. |
epenet
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.
Conflicts need resolving
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
…eData to keep backwards compability
6626098 to
073a73e
Compare
| @classmethod | ||
| def from_raw(cls, data: str) -> Self: | ||
| """Return a RawTypeData object.""" | ||
| return cls(raw=data) |
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.
Would it not make sense to decode it with base64.b64decode?
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.
Preffered but high risk to hit the 256 limit. Maybe later if subtype is needed it make sense to decode 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.
Then I fail to see the value of the current base64 string.
Please adjust the PR description to highlight how it may be useful.
A sensor "just because it's available" is not generally approved unless there is a valid scenario.
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.
Here is an example of usage https://github.com/FredrikM97/mealplan-card
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.
Overall it feels wrong to force raw or json types to by ElectricityTypeData
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.
If that is the case then it sounds reasonable to use a service if possible. Still think we should change the ElectricityTypeData somehow to be more generic.
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 agree - but I'm not sure if that will be approved if there is no other BaseType (that is why I suggested exposing other sensors)
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.
What about keeping base class and separate the Mealplan to a service? Then it will be easier for future to add new typeData for raw/json.
If you have an idea how to expose a list of sensors for up to 10 schedules without overcomplicate it I'm all ears.
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 don't think you need to add all schedules...
But maybe showing four sensors has value:
- Total schedules: 5
- Active schedules: 2
- Today feed amount: 15 g
- Weekly feel amount: 150 g
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.
How do we decide what fields exist? If you check the Mealplan card there is profiles depending on device. Since base64 does not map the same for all devices. Can't come up with a good workaround for that.
Proposed change
I have a sensor that provides a list of objects representing data from my pet feeder. However, the current implementation only supports ElectricityTypeData as the allowed TypeData for raw input. If no subkey is specified, the data is skipped and the sensor state becomes unknown. Additionally, ElectricityTypeData is not capable of decoding the pet feeder's data format. Therefore, a more flexible and extensible implementation is necessary to support custom data types like the pet feeder.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: