Added 'sensorTemperature' and 'iFeel' to IRac (common)

[mbronk]

Adds support for sensorTemperature (a.k.a. roomTemp, iFeel, ...) incl. temp. source selection to IRac.

For: #1912

This PR is a standalone part#3 of changes detached from: #1913.

• Changes in 4065940 (new param + Argo WREM3 support) are confirmed working,
• Changes in 0abf6cc are opportunistic and not tested in practice (based solely on ...roomTemp... existence in other protocols.
  • @crankyoldgit - I've added these as these seem to match the interface and I believe are in line with your comment linked below. That said, these are "naive" and I'm unable to tell if they are complete or "safe".
    I'm happy to pull these and submit only the "Argo" part which I can confirm working. On the other hand - if you'd like to get the support lib-wide and I failed at updating sth, I'd welcome co-authoring this change set.

---

Relevant discusion:

• [do not merge / consolidated PR] Added support for Argo WREM-3 A/C remote protocol #1913 (comment)

[NiKiZe]

I have minor reservations due to the code size increment compared to how useful this might be.

However that is something that can be resolved in other ways if needed.

[crankyoldgit]

Sorry for all the comments. Looks pretty good except for my comments.

[crankyoldgit]

typo: s/cab/can/

[crankyoldgit]

Why are you increasing the desired temp here by 0.5 degrees?

[crankyoldgit]

See later comments about this. Not commenting here due to proximity to the other comment.

[crankyoldgit]

Why are you (seemingly always) setting the sensor temp half a degree higher than the requested sensorTemp value? This makes little sense to me.
Also, it seems like a lot of duplicate code (for every time you do it) in each method.
This goes for almost all occurances of it.

[crankyoldgit]

Also, if it is to make the sensor temp be higher than the desired temp so the A/C operates, that's potentially a failure.
i.e. The A/C units might not decide to operate within +/-2 deg or something. Also, you may be assuming "Cool" mode. i.e. If the unit is in "Heat" mode, you'd want the opposite in that approach.

In principle, these methods/procedures/etc should deliver the results they have been instructed to do, not change the behavior unless needed. That allows decissions, changes, and operational logic to be done at a higher (and maybe more common) level.

But this is all a guess for why you're adding half a degree ... shrug.

[mbronk]

tl;dr;

Will update to std::round(). Please check if this would be OK with your direction, or if it should be std::trunc() || static_cast<uint8_t>() instead?

BTW. I'd highly recommend to consider enabling -Wconversion or at least -Wfloat-conversion in UT compiler options someday, so that these narrowing conversions are always explicit.
I'm not doing this now, as it will/does yield a ton of warnings...

Verbose

Why are you (seemingly always) setting the sensor temp half a degree higher than the requested sensorTemp value?

I'm merely converting float->uint8_t (with rounding). That is:

0.1 -> 0
0.5 -> 1
0.9 -> 1
1.4 -> 1
etc.

Given that it wasn't immediately apparent, signals to me that it reads poorly, I now see I should have used std::round() or even std::nearbyint() (see here for how they differ) for it, esp. since my naive method doesn't handle negative numbers too intuitively (i.e. -1.3 would become a 0).

---

if it is to make the sensor temp be higher than the desired temp so the A/C operates

No, not really. I only wanted to capture the gist of the interface (which operates on float).

The A/C units might not decide to operate within +/-2 deg or something.

This thinking is the reasoning behind a more "natural" rounding of values.
While it is indeed the current implementation of degrees (for all int-based AC temp. protocols I've seen across the lib), to truncate the decimal part... I fear that a higher-precision temp sensor (operating with, say 2 digit precision) may encounter the following:

mode:                COOL
degrees:             21 [°C]
sensorTemperature:   21.99 [°C]

So, if I left it as-is, it would be int(21) == 21 == int(21.99) and would likely cause the device to sit idle (while it should start cooling, give or take its temp hysteresis configuration), which is why I broke convention of truncating (this wasn't an explicit one, though... so I'm not sure how deliberate that was).

[mbronk]

Also, it seems like a lot of duplicate code (for every time you do it) in each method.

std::round would (kind-of) take care of it (fun fact... it will be more characters to write than the +0.5 :) but much more proper ).

I'd argue though the static _cast<>() should stay there and while this is repeated in multiple places... is not boilerplate (I'm of the opinion a narrowing cast should always be very explicit, so that it signals the "I know what I'm doing" to future maintainers :) ).
Creating a custom "i8round()" function (following boost's iround [ref]) to get rid of the static_cast and implement overflow checking... is also possible, but I think it would be an overkill for this (there's no overflow check today for degrees, so probably is a YAGNI item).

[crankyoldgit]

Change comment to:
// Set the sensor temp to the desired (normal) temp. or make it clearer/more obvious.

This confused me for a bit as I read the comment, and not the code. I thought you were doing:
ac->setSensorTemp(sensorTemp); here instead of degrees based on the comment.

[mbronk]

Will update as suggested.

• Disclamer: I copied this as-is from what used to be line 2242. To be honest, I don't know exactly why it was being done (seems counterintuitive: always sets desired == actual so I'd assume the A/C may decide to do nothing in this case).
  I have left it however, as it was there in the original code. If you know if this can be removed, I'd happily delete this.

On the flip side, I have even debated if not to apply this pattern globally, e.g.:

stdAc::state_t IRac::cleanState(const stdAc::state_t state) {
    stdAc::state_t result = state;
    // (...)
    if (state.sensorTemp == kNoTempValue) result.sensorTemp = state.degrees;   //< HERE
    return result;

But finally decided against it as too risky.

[crankyoldgit]

Why the +0.5?

[mbronk]

Sorry for all the comments. Looks pretty good except for my comments.

No worries. I appreciate all the comments... just may not be able to digest them fast enough :)

By the way, the discussion on +0.5 rounding made me realize... that even though the target/sensor temperature values are floats and can be negative... they're often pushed into a uint8_t interface, so... become really high uint's (-1 => 255).

Not sure if this is a problem (I know some A/C protocols offset the value by a few units, but not sure if any proto. - e.g. for heat pumps - allows the target/ambient temperature to be negative).
If yes, this would require changing the types used to sth like int16_t and allow downcast to uint8_t only after the offset is applied (or do float all the way, until the very last mile).

Not making this change proactively though, as:

1. It's likely not needed anyway
2. It will take lib-wide update, and my intent is to contain changes

[mbronk]

Hi @crankyoldgit,
Any updates?