muart-group / esphome

ESPHome fork for development of the mitsubishi_uart component. Check out the documentation for more info:
https://muart-group.github.io/
10 stars 1 forks source link

Validate UART config to prevent misconfiguration errors #47

Closed paravoid closed 5 months ago

paravoid commented 5 months ago

Use uart.final_validate_device_schema() to validate the passed UARTs:

Sammy1Am commented 5 months ago

Requiring rx/tx is definitely a good idea (and provides a bit of sanity check in general for the config).

It seems pretty super-unlikely that anyone would need to connect to something not 1/8/EVEN, but in the unlikely event that someone wants to try, does this validation entirely prevent moving forward? Is there a way to make this a warning instead of an error, or a way to force compilation ignoring validation?

Edit: I mean if they really wanted to try they can obviously checkout the repo and remove the validation code-- just asking so I know what the options are.

paravoid commented 5 months ago

It seems pretty super-unlikely that anyone would need to connect to something not 1/8/EVEN, but in the unlikely event that someone wants to try, does this validation entirely prevent moving forward? Is there a way to make this a warning instead of an error, or a way to force compilation ignoring validation?

Not that I know of...

KazWolfe commented 5 months ago

Allow setting the baud rate freely, as we know this varies between models

Baud rate will either be 2400 or 9600 as well. We can lock to one of these two settings.

Allow setting the baud rate freely, as we know this varies between models

I think that if (somehow) someone has a unit that doesn't comply with these settings, it's okay to error out until we can step in to figure out what's going on.

Sammy1Am commented 5 months ago

Well, I couldn't easily find a way to use cv.one_of(2400,9600,int=True) with that validation function. @paravoid , if you know a way to add that, I agree with @KazWolfe that we might as well lock that value down as well.

However if there's no easy way to do that, I'm happy to merge as is.

paravoid commented 5 months ago

Looking at the UART final_validate_device_schema() function, it's clear that's not something it supports. We could add it upstream, however...

geoffdavis/esphome-mitsubishiheatpump says Most systems use the default value of 4800 baud, but some use 9600. Default: 4800. (Mine operates at 2400, which isn't mentioned there at all). So perhaps there are three speeds widely deployed? It sounds like this is not as well understood?

Generally speaking, while I am worried about users forgetting to configure parity, I'm not too worried about them forgetting to set the baud rate. It's clear that this is a knob folks will need to experiment with, and the value will depend on the unit they have. Limiting the options there isn't bad per se, but I don't think it's super important either.