MIELHVAC fix(miel_hvac): memory leak, remote temp logic, code deduplication#24652
Closed
grzegorz914 wants to merge 21 commits intoarendst:developmentfrom
Closed
MIELHVAC fix(miel_hvac): memory leak, remote temp logic, code deduplication#24652grzegorz914 wants to merge 21 commits intoarendst:developmentfrom
grzegorz914 wants to merge 21 commits intoarendst:developmentfrom
Conversation
- added energy sensors POWER, ENERGY TOTAL, FREQUENCY - other changes and improvements
- added all available mode mapping to Home Assistant
…into development
- Description of Changes Temperature fields as numbers: Updated all temperature outputs (SetTemperature, RoomTemperature, OutdoorTemperature) to be serialized as numeric values (float) in JSON instead of strings. This ensures proper numeric handling in clients and Home Automation platforms. - Energy, power, and operation time as numbers: Converted PowerUsage, Energy, OperationTime, and timer fields (TimerOn, TimerOff, TimerOnRemaining, TimerOffRemaining) to numeric values in JSON. Previously these were serialized as strings. - JSON output consistency: All numeric fields now correctly appear as numbers or floats, while mapped fields (modes, fan speed, swing, air direction, prohibit) remain strings. - Renamed 'Temp' to 'SetTemperture' in 'HVACSettings' - Renamed 'Power" to 'PowerState' for SENSOR because of use 'Power' for ENERGY
…into development
Changed Commands: - 'HVACRemoteTemp' to 'HVACSetRemoteTemp' - 'HVACRemoteTempClearTime' to 'HVACSetRemoteTempClearTime' Value of ModeStage: - changed 'manual' to 'direct' - available values are: 'direct', 'auto_heat', 'auto_dry', 'auto_fan', 'auto_leader' Other small improvements for Home Assistant Climate Control
- fix remote temperature update - refactor remote temperature code - added remote temperature to sensor output
## fix(miel_hvac): memory leak, remote temp logic, code deduplication ### Summary This PR fixes three bugs (one causing a spurious serial write on every boot), corrects a wrong lookup table in `HVACSetPurify`, removes ~80 lines of duplicated JSON-building code, and cleans up misleading comments. --- ### Bug fixes **Memory leak in `miel_hvac_pre_init()` (#1)** When `sc_serial->begin()` failed the code executed `goto del`, which deleted the serial object but never reached `free(sc)` — the `free:` label existed but was never jumped to. `sc` leaked on every failed initialisation. Fixed by replacing the `goto` chain with an explicit `delete + free + return`. ```c // before del: delete sc->sc_serial; free: // dead label — never reached free(sc); // after delete sc->sc_serial; free(sc); return; ``` **`remotetemp_clear` semantics were inverted (#2)** The variable was named `remotetemp_clear` but acted as an "active" flag: setting it to `true` meant the sensor was active, yet it also armed the auto-clear timer and was published to MQTT as `"on"`. Renamed to `remotetemp_active` with corrected semantics throughout (`true` = remote temperature override is active, `false` = no override). **Spurious CLR frame sent on every boot (#3)** The auto-clear condition included `remotetemp_last_call_time == 0`, and the old `remotetemp_clear` was initialised to `true`. Combined, this caused a remote-temperature CLR packet to be sent to the HVAC unit on every boot before any sensor had registered a value. Fixed by initialising `remotetemp_active = false` and removing the `== 0` branch from the timer check. ```c // before if (remotetemp_clear && (remotetemp_last_call_time == 0 || // fires immediately on boot millis() - remotetemp_last_call_time > remotetemp_auto_clear_time)) // after if (remotetemp_active && millis() - remotetemp_last_call_time > remotetemp_auto_clear_time) ``` --- ### Correctness fixes **`HVACSetPurify` used the wrong lookup table (#5)** `miel_hvac_cmnd_setpurify()` was passing `miel_hvac_airdirection_map` to `miel_hvac_map_byname()`, so the command accepted `indirect/direct/even/off` instead of `on/off`. Restored and enabled `miel_hvac_purify_map` with values `{0x00, "off"}` / `{0x08, "on"}` (verify `0x08` against your hardware model). **Misleading hex comments in `miel_hvac_cmnd_setairdirection()` (#9)** The `switch` cases had comments like `// 0x80`, `// 0xaa` which referred to `widevane` wire values, not to the `airdirection` `#define` values (`0x00`–`0x03`). Removed the misleading annotations and replaced them with a single explanatory comment describing the i-see sensor / widevane relationship. --- ### Refactoring **Deduplicated settings JSON builder (#6)** `miel_hvac_publish_settings()` and `miel_hvac_sensor()` contained ~80 lines of near-identical code building the settings JSON payload. Extracted into a new static helper `miel_hvac_append_settings_json()`. Both callers now invoke the helper; any future field additions require a single change. --- ### Minor improvements - **`htons()` usage on `flags` documented** — added a comment explaining why network byte-order conversion is used (wire protocol is big-endian; ESP8266/ESP32 is little-endian). - **`operationtime` documented as unverified** — added a comment clarifying the 3-byte big-endian layout and noting that the unit (presumed minutes) has not been confirmed against hardware. - **Local `char` buffers in timer/status JSON consolidated** — four separate `char[33]` declarations replaced with a single reused `buf[33]`. --- ### Reverted: length check in `miel_hvac_input_connected()` An earlier version of this PR added a payload-length guard to `miel_hvac_input_connected()` comparing against `sizeof(miel_hvac_msg_connect)` (2 bytes). This was incorrect: the `CONNECTED` response (type `0x7a`) sent by the HVAC unit is an independent protocol packet whose length is unrelated to the connect request payload. The check rejected every valid response, keeping `sc_connected` permanently `false` and preventing all subsequent settings/sensor requests — resulting in an always-empty `MiElHVAC:{}` sensor object. The guard has been removed; parser-level checksum and length validation is sufficient. --- ### Testing Tested on ESP32-S3 with a Mitsubishi MSZ-LN series unit. Verified: - `MiElHVAC` sensor object populated within a few seconds of boot - no CLR packet on boot - `HVACRemoteTemp 22.5` sets and auto-clears correctly after the configured timeout - `HVACSetPurify on/off` accepted and transmitted correctly - `HVACSettings` MQTT topic content unchanged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
This PR fixes three bugs (one causing a spurious serial write on every boot), corrects a wrong lookup table in
HVACSetPurify, removes ~80 lines of duplicated JSON-building code, and cleans up misleading comments.Bug fixes
Memory leak in
miel_hvac_pre_init()(#1)When
sc_serial->begin()failed the code executedgoto del, which deleted the serial object but never reachedfree(sc)— thefree:label existed but was never jumped to.scleaked on every failed initialisation. Fixed by replacing thegotochain with an explicitdelete + free + return.remotetemp_clearsemantics were inverted (#2)The variable was named
remotetemp_clearbut acted as an "active" flag: setting it totruemeant the sensor was active, yet it also armed the auto-clear timer and was published to MQTT as"on". Renamed toremotetemp_activewith corrected semantics throughout (true= remote temperature override is active,false= no override).Spurious CLR frame sent on every boot (#3)
The auto-clear condition included
remotetemp_last_call_time == 0, and the oldremotetemp_clearwas initialised totrue. Combined, this caused a remote-temperature CLR packet to be sent to the HVAC unit on every boot before any sensor had registered a value. Fixed by initialisingremotetemp_active = falseand removing the== 0branch from the timer check.Correctness fixes
HVACSetPurifyused the wrong lookup table (#5)miel_hvac_cmnd_setpurify()was passingmiel_hvac_airdirection_maptomiel_hvac_map_byname(), so the command acceptedindirect/direct/even/offinstead ofon/off. Restored and enabledmiel_hvac_purify_mapwith values{0x00, "off"}/{0x08, "on"}(verify0x08against your hardware model).Misleading hex comments in
miel_hvac_cmnd_setairdirection()(#9)The
switchcases had comments like// 0x80,// 0xaawhich referred towidevanewire values, not to theairdirection#definevalues (0x00–0x03). Removed the misleading annotations and replaced them with a single explanatory comment describing the i-see sensor / widevane relationship.Refactoring
Deduplicated settings JSON builder (#6)
miel_hvac_publish_settings()andmiel_hvac_sensor()contained ~80 lines of near-identical code building the settings JSON payload. Extracted into a new static helpermiel_hvac_append_settings_json(). Both callers now invoke the helper; any future field additions require a single change.Minor improvements
htons()usage onflagsdocumented — added a comment explaining why network byte-order conversion is used (wire protocol is big-endian; ESP8266/ESP32 is little-endian).operationtimedocumented as unverified — added a comment clarifying the 3-byte big-endian layout and noting that the unit (presumed minutes) has not been confirmed against hardware.charbuffers in timer/status JSON consolidated — four separatechar[33]declarations replaced with a single reusedbuf[33].Testing
Tested on ESP32-S3 with a Mitsubishi MSZ-LN series unit. Verified:
MiElHVACsensor object populated within a few seconds of bootHVACRemoteTemp 22.5sets and auto-clears correctly after the configured timeoutHVACSetPurify on/offaccepted and transmitted correctlyHVACSettingsMQTT topic content unchangedChecklist:
NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass