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 binary sensor types #244

Merged
merged 3 commits into from
Oct 28, 2024
Merged

Conversation

abstractionnl
Copy link

Add support for marking sensors as binary ON/OFF sensors.

@meatpiHQ
Copy link
Owner

@jay-oswald can you please review schema.json

@jay-oswald
Copy link
Collaborator

Looks good.

Though if were adding type I think we need to either make it required, or default it in the merge.js code (since 90%+ will be sensor)

@abstractionnl
Copy link
Author

Looks good.

Though if were adding type I think we need to either make it required, or default it in the merge.js code (since 90%+ will be sensor)

I made this property optional during parse, so it does not need to be required.

@jay-oswald
Copy link
Collaborator

Looks good.
Though if were adding type I think we need to either make it required, or default it in the merge.js code (since 90%+ will be sensor)

I made this property optional during parse, so it does not need to be required.

Yeah, but that can just make things more confusing it means the default has to be applied in the WiCAN FW, easier to just apply it at this level instead. We can leave it optional for each profile, but then I'd prefer it be defaulted in the merge.js file, so WiCAN can rely on it being there.

The C code is low level, and very limited resources, so the more we can do in this JS the less that has to be done in C

@abstractionnl
Copy link
Author

Looks good.
Though if were adding type I think we need to either make it required, or default it in the merge.js code (since 90%+ will be sensor)

I made this property optional during parse, so it does not need to be required.

Yeah, but that can just make things more confusing it means the default has to be applied in the WiCAN FW, easier to just apply it at this level instead. We can leave it optional for each profile, but then I'd prefer it be defaulted in the merge.js file, so WiCAN can rely on it being there.

The C code is low level, and very limited resources, so the more we can do in this JS the less that has to be done in C

A null check is probably less load for the ESP than having to parse the element and compare it to another string. It also removes some redundant bytes from the json file which should also reduce storage and memory pressure.

@meatpiHQ meatpiHQ merged commit 6942d03 into meatpiHQ:main Oct 28, 2024
1 check passed
@abstractionnl abstractionnl deleted the binary-sensors branch October 28, 2024 13:54
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.

3 participants