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

small changes #1306

Merged
merged 11 commits into from
Sep 25, 2023
Merged

small changes #1306

merged 11 commits into from
Sep 25, 2023

Conversation

proddy
Copy link
Contributor

@proddy proddy commented Sep 23, 2023

@MichaelDvP could you look at the TODO in thermostat.cpp

// TODO: should this be >= 87 to avoid out of bounds?
if (telegram->message_length + telegram->offset >= 92 && telegram->offset <= 87) {
    char data[sizeof(wwVacation_)];

@proddy proddy requested a review from MichaelDvP September 23, 2023 16:44
@@ -843,6 +843,8 @@ void Thermostat::process_RC35wwTimer(std::shared_ptr<const Telegram> telegram) {
if (telegram->type_id != 0x38) {
return;
}

// TODO: should this be >= 87 to avoid out of bounds?
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is the right check, we make sure that all offsets 87-92 are in the telegram. (e.g. read 10 38 90 gives offsets 90-117, so 87, 88, 89 are missing, o processing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sorry, not sure what I was thinking

@@ -331,7 +331,7 @@ bool DeviceValue::get_custom_min(int16_t & val) {
if (fahrenheit) {
v = (v - (32 * (fahrenheit - 1))) / 1.8; // reset to °C
}
if (max > 0 && v > max) {
if (max > 0 && v > (int16_t)max) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be wrong to shorten max, if you want to compare identical types do
if (max > 0 && v > 0 && (uint32_t)v >max) {

Maybe it's much easier to set max as int32_t and use 31 bit range and handle the inputs for min/max also in int32_t, only shorten min to int16_t for storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, this was just so the code would compile on my MacBook - the compiler would throw an error. I'll go for option 1 or look at a compiler option to ignore these checks.

@proddy proddy merged commit 5bc3bd2 into emsesp:dev Sep 25, 2023
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.

2 participants