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

Configuration endpoint for local configuration #86

Closed
wants to merge 2 commits into from

Conversation

austvik
Copy link
Contributor

@austvik austvik commented Mar 16, 2024

This makes it possible to read and write configuration (like ledMode and mqtt url) and trigger different calibrations locally.

It is a draft meant to spark a discussion, because while it does update the configuration on the device, the setting only last until they are updated (and overwritten) from the cloud (for an average of 7.5 seconds).

This makes it possible to read and write configuration (like ledMode and
mqtt url) and trigger different calibrations locally.

It is a draft meant to spark a discussion, because while it does update
the configuration on the device, the setting only last until they are
updated (and overwritten) from the cloud (for an average of 7.5 seconds).
@airgradienthq
Copy link
Owner

Thank you for this. I will check with our home assistant developer if that would be a good approach for our upcoming HA integration.

@austvik
Copy link
Contributor Author

austvik commented Mar 21, 2024

Hi, @airgradienthq, did you figure out something here?

Copy link

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

I am lacking some knowledge of the internals and the complete use case of every field in the body. But I think I have some points that would be useful to convey.

If I interpret the code correctly we do need to know the previous state of the device to do a proper call. If for example my first request is

{
    "co2CalibrationRequested": true
}

That property will turn on, but when I send the request

{
    "tempFormat": "c"
}

This property is applied, but co2CalibrationRequested is set to False since it wasn't in the json.

So this means we have to keep track of the state before sending a request to avoid having a side effect.

And this doesn't have to be a bad thing, since it's a local device, so there's not that big of a deal to do a request before sending, but it would maybe be better if the API was a bit more stateless.

Tldr, The integration would be able to work with any, but I think a more stateless approach would make the API more predictable.

@austvik
Copy link
Contributor Author

austvik commented Mar 24, 2024

Hi @joostlek! Good point. I updated the patch to address the comment about not touching the values if they are not set.

Two of the values are "triggers" (co2CalibrationRequested, ledBarTestRequested), which will be reset to false after the action they request is done.

I think the good thing about this patch is that is uses the same code to parse settings from local automation as it does from the cloud API - ensuring that the same functionality is available both places.

The bad thing is that the API itself isn't documented outside the firmware code, and e.g. how it threats Country and Farenheit/Celsius isn't perfect. Since it (now) only deals with values that is set, it is possible to safely introduce "tempFormat" in addition, so I think it is an OK tradeoff for the good parts of all settings being available.

The ugly part of the API is that the settings aren't persisted since the cloud API sends all the configuration all the time, so if you change a setting it is only applied until settings are downloaded from the cloud again (every 15 seconds). I think that needs a design - either the device uploads settings to the cloud, or there is a (persisted) setting to not download values from the cloud or something like that.

@austvik
Copy link
Contributor Author

austvik commented Apr 4, 2024

I see this is being addressed in a similar but better way in the source code.

@austvik austvik closed this Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants