#OpenDisplay PR
bleak summit
they can split off services
but I withheld me from reviewing it knowing that @quick badger already has opinions on it
or at least, he helped them at the beginning
rancid granite
Yeah they have been talking to Josef
bleak summit
I know, Josef has been talking to me 😂
rancid granite
majority of size is tests btw Frenck
buoyant bison
majority of size is tests btw Frenck
I know, just saying first response is: That even for a first PR the line count is above average 😄
rancid granite
these guys know what they are doing. Have been very impressed with their coding, also the firmware
buoyant bison
I appreciate your judgement; as said, first look without considering contents from my end.
rancid granite
We now have such flexible translations that we can make a great UI, that is not going to be great for YAML users. I see that in this PR. The service takes an integer. The selector in services.yaml puts in 0, 1, 2, 3 etc. Then strings.json gives normal names to the numbers.
I feel like that's hacky, and we should just accept strings that represent at least the value it means, so YAML users don't suffer. What do people think?
https://github.com/home-assistant/core/pull/164048/changes#r2861652215
I left a first round of review
But not sure if I have time to followup on it, so feel free to take over
(too busy working on my own new integration)
bleak summit
We now have such flexible translations that we can make a great UI, that is not ...
yes, states and options should have their name there
rancid granite
ok, yeah left that as a comment too
quick badger
So we are talking about the same thing, this is what they were discussing with me
Why an image input entity instead of a service?
- allows you to use entity services
- allows you to view the rendered image without being next to the device and waiting for the refresh to occur on device side
rancid granite
I think that's smart as it also allows you to debug. But do we still care the image persist across restarts ? Feels unnecessary
Otherwise it's fine
quick badger
I think that's smart as it also allows you to debug. But do we still care the im...
if you don't, the images on the epapers will reset if you restart HA, which is not what users would want (at least I wouldn't)
buoyant bison
Would be even nicer if we got the image from the device and reflected reality
quick badger
for always we discussed that, but I felt like it's weird if we mix input/output, so we should have a clear separation
buoyant bison
We do not need to store input
We never do that
Home Assistant reflects state of a device, that is our purpose in general
quick badger
also I think the firmware doesn't allow getting the image back from the device currently
buoyant bison
In that case, I do not understand the image entity in the above diagram
By the sound of it, it is virtual (as it is not provided by the device), which is not something we allow
quick badger
I wouldn't say it's virtual since it's representing the physical device. I consider it similar to a number entity in an integration with assumed_state where you set something on the entity which is then written to the device
they were thinking about adding the get_image endpoint to the firmware, so that's something that can be addressed if needed
buoyant bison
Sounds weird, we do not add text entities to all notification capable integrations with the last send message either
In the end, it is not provided by the device 🤷♂️
bleak summit
We do not need to store input
I mean we do when we do optimistic state/assumed state
buoyant bison
sure, by the intent is different
rancid granite
also I think the firmware doesn't allow getting the image back from the device c...
You can't read e-ink screens programmatic, so it will never be possible
The firmware is made in such a way that it pushes the pixels directly to the screen, so the device requires less PSRAM
Frenck is right that it's not needed for the integration to work
It can just be a service
Since we sent images from the media browser, a user can go there to see what was sent
quick badger
the thing is you also want to send non-media browser images
like custom dashboards
especially when we are talking about tags and not epaper frames
for a first thing we can start with image, but we should consider that in the architecture
rancid granite
quick badger
the nice thing about an entity service is that you can auto target areas etc.
rancid granite
This is what I shared with them as my vision
quick badger
ya I saw that
rancid granite
the nice thing about an entity service is that you can auto target areas etc.
Sensor entity with last upload and make that target of service?
buoyant bison
Doesn't open display provide any form of information?
PS: The entity service path would be odd, considering it is not really setting something on the entity
A service action that targets a device is the right path here
I do agree, it would be nice to target an area, but that is not really a thing to be solved on an integration level (and not something we should workaround in an integration level).
rancid granite
Doesn't open display provide any form of information?
No. It's meant to be in deep sleep most of the time to get years of battery
buoyant bison
right
We should consider making the target selector capable of "devices" (but that is something a bit out of scope for here. But I like the thought of it)
rancid granite
Yeah or device selector to accept areas that are resolved
buoyant bison
right, but the target selector and its UX is pretty sweet now
would be nice to re-use that
quick badger
We should consider making the target selector capable of "devices" (but that is ...
wasn't that explicitly removed by Erik a while ago
buoyant bison
That was the device filter; that is something else
Right now, a target will always resolve to entities in the end
no matter of you select a area, device, label, etc
the outcome internally are always entities.
The suggestion than is, to allow defining the needed outcome from the target selector (e.g., devices to target, instead of entities as it does now)
quick badger
ah that you mean