-
Notifications
You must be signed in to change notification settings - Fork 2
Bugfix: ComfoClime negativ temperature #22
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
Bugfix: ComfoClime negativ temperature #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where negative outdoor temperatures from the ComfoClime API were being displayed incorrectly (e.g., 6000°C instead of negative values). The fix introduces a new _decode_raw_value function to handle signed/unsigned integer conversion with two's complement, and applies it automatically to temperature fields in the dashboard data.
Key Changes:
- Added
_decode_raw_valuefunction to handle signed integer decoding with auto-scaling detection - Modified
get_dashboard_datato automatically decode temperature fields (those ending with "Temperature") - Updated comments in
set_property_for_devicefrom German to English
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SCALED_VALUE_THRESHOLD = 1000 # Threshold for already scaled values (e.g., temperature fields) | ||
|
|
||
|
|
||
| def _decode_raw_value(raw, factor=0.1, signed=True, byte_count=None): | ||
| """Decode raw sensor values with auto-scaling and signed/unsigned conversion. | ||
|
|
||
| Note: The heuristic 'Small values (< 1000) → return as-is (already scaled)' is intended for temperature fields, | ||
| which are typically already scaled by the API. For other sensor types (e.g., fan speed, percentage values), | ||
| this may not be correct and could result in incorrect decoding (e.g., a raw value of 200 may need decoding). | ||
| Large values (≥ 1000) → decode as signed/unsigned int and apply factor. | ||
|
|
||
| Args: | ||
| raw: Raw value to decode | ||
| factor: Scaling factor | ||
| signed: Whether to interpret value as signed (default True) | ||
| byte_count: Number of bytes (1 or 2), if known | ||
| """ | ||
| if raw is None: | ||
| return None | ||
|
|
||
| try: | ||
| # Already scaled values pass through | ||
| if abs(raw) < SCALED_VALUE_THRESHOLD: | ||
| return float(raw) | ||
|
|
||
| # Apply two's complement for signed conversion | ||
| if 0x80 <= raw_int <= 0xFF: # 1-byte signed | ||
| raw_int -= 0x100 | ||
| elif 0x8000 <= raw_int <= 0xFFFF: # 2-byte signed | ||
| raw_int -= 0x10000 |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The heuristic using SCALED_VALUE_THRESHOLD = 1000 is a fragile approach. As noted in the docstring, this assumes temperature fields are already scaled, but may fail for other sensor types. This creates a maintainability issue where:
- The threshold is a magic number without clear justification
- The logic is tightly coupled to temperature behavior
- Future sensor types could be mishandled
Consider either:
- Making the threshold configurable per sensor type, or
- Using explicit metadata to indicate whether a value is already scaled, or
- Documenting which specific API response fields are pre-scaled vs. raw
| SCALED_VALUE_THRESHOLD = 1000 # Threshold for already scaled values (e.g., temperature fields) | |
| def _decode_raw_value(raw, factor=0.1, signed=True, byte_count=None): | |
| """Decode raw sensor values with auto-scaling and signed/unsigned conversion. | |
| Note: The heuristic 'Small values (< 1000) → return as-is (already scaled)' is intended for temperature fields, | |
| which are typically already scaled by the API. For other sensor types (e.g., fan speed, percentage values), | |
| this may not be correct and could result in incorrect decoding (e.g., a raw value of 200 may need decoding). | |
| Large values (≥ 1000) → decode as signed/unsigned int and apply factor. | |
| Args: | |
| raw: Raw value to decode | |
| factor: Scaling factor | |
| signed: Whether to interpret value as signed (default True) | |
| byte_count: Number of bytes (1 or 2), if known | |
| """ | |
| if raw is None: | |
| return None | |
| try: | |
| # Already scaled values pass through | |
| if abs(raw) < SCALED_VALUE_THRESHOLD: | |
| return float(raw) | |
| # Apply two's complement for signed conversion | |
| if 0x80 <= raw_int <= 0xFF: # 1-byte signed | |
| raw_int -= 0x100 | |
| elif 0x8000 <= raw_int <= 0xFFFF: # 2-byte signed | |
| raw_int -= 0x10000 | |
| # Mapping of sensor types to scaling metadata | |
| SENSOR_TYPE_METADATA = { | |
| # Example: 'temperature': {'already_scaled': True}, | |
| # 'fan_speed': {'already_scaled': False, 'factor': 0.1, 'signed': True}, | |
| # Add more sensor types as needed | |
| 'temperature': {'already_scaled': True}, | |
| 'fan_speed': {'already_scaled': False, 'factor': 0.1, 'signed': True}, | |
| 'percentage': {'already_scaled': False, 'factor': 1.0, 'signed': False}, | |
| # etc. | |
| } | |
| def _decode_raw_value(raw, sensor_type='temperature', factor=0.1, signed=True, byte_count=None): | |
| """Decode raw sensor values using sensor type metadata for scaling and signed/unsigned conversion. | |
| Args: | |
| raw: Raw value to decode | |
| sensor_type: Type of sensor (e.g., 'temperature', 'fan_speed', 'percentage') | |
| factor: Scaling factor (overrides metadata if provided) | |
| signed: Whether to interpret value as signed (overrides metadata if provided) | |
| byte_count: Number of bytes (1 or 2), if known | |
| Returns: | |
| Decoded value as float, or None if decoding fails. | |
| """ | |
| if raw is None: | |
| return None | |
| # Get metadata for sensor type | |
| metadata = SENSOR_TYPE_METADATA.get(sensor_type, {}) | |
| already_scaled = metadata.get('already_scaled', False) | |
| factor = metadata.get('factor', factor) | |
| signed = metadata.get('signed', signed) | |
| try: | |
| if already_scaled: | |
| return float(raw) | |
| raw_int = int(raw) |
| # Auto-decode only temperature fields (ending with 'Temperature') | ||
| return { | ||
| key: _decode_raw_value(val, factor=0.1) | ||
| if isinstance(val, (int, float)) and key.endswith("Temperature") | ||
| else val | ||
| for key, val in data.items() | ||
| } |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The auto-decoding in get_dashboard_data only applies to fields ending with "Temperature", but the _decode_raw_value function is designed with a broader heuristic (the SCALED_VALUE_THRESHOLD check). This creates confusion about the function's intended scope. Consider either:
- Renaming
_decode_raw_valueto_decode_temperature_valueto make its purpose clearer, or - Removing the temperature-specific heuristic from the function and handling it at the call site
| if 0x80 <= raw_int <= 0xFF: # 1-byte signed | ||
| raw_int -= 0x100 | ||
| elif 0x8000 <= raw_int <= 0xFFFF: # 2-byte signed | ||
| raw_int -= 0x10000 |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable raw_int is used before it's defined. On line 32, the code checks if abs(raw) < SCALED_VALUE_THRESHOLD, but then on lines 36-39 it tries to use raw_int which hasn't been assigned yet. You need to add raw_int = int(raw) before line 32 or at least before line 36.
| # Apply two's complement for signed conversion | ||
| if 0x80 <= raw_int <= 0xFF: # 1-byte signed | ||
| raw_int -= 0x100 | ||
| elif 0x8000 <= raw_int <= 0xFFFF: # 2-byte signed | ||
| raw_int -= 0x10000 | ||
|
|
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block applies two's complement conversion to raw_int, but this happens before the variable is converted to an integer in the logic flow. Additionally, this entire block (lines 35-39) appears redundant because the same conversion is applied again later (lines 48-53). This duplicate logic should be removed to avoid confusion and potential double-conversion errors.
| # Apply two's complement for signed conversion | |
| if 0x80 <= raw_int <= 0xFF: # 1-byte signed | |
| raw_int -= 0x100 | |
| elif 0x8000 <= raw_int <= 0xFFFF: # 2-byte signed | |
| raw_int -= 0x10000 |
| # Determine byte_count if not provided | ||
| if byte_count is None: | ||
| if raw_int <= 0xFF: | ||
| byte_count = 1 | ||
| elif raw_int <= 0xFFFF: | ||
| byte_count = 2 |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The byte_count determination logic uses raw_int which hasn't been defined yet at this point in the code. The variable needs to be initialized before this check. Additionally, the condition if raw_int <= 0xFF will always be true for negative values after two's complement conversion has been applied (which shouldn't have happened yet anyway due to the missing initialization).
|
Which Sensor exactly is pushing wrong data? The API is already capable to handle negative values. We should Look for the bug in that code instead of building new functions to handle that. The scale factor should Not be guessed depending on values, as there are telemetries, that output high values without the need to be scaled. All sensors must be defined with the correct scale and sign config. |
|
Could you provide the output of your Dashboard JSON? As this isn't happening on my side. |
Okay, I understand. Then the How else do you think this could be implemented? What's also interesting is that when I retrieve the value from my ComfoAirQ via http://%IP%/device/%UUID_ComfoAirQ%/telemetry/276, the following JSON is returned: This would match my observation from above. |
|
Wow, that's pretty fucked up in Zehnders Firmware. Can you record for a few values the output of the Dashboard and what the telemetry answers? The scale factor isn't changing, they just ignore the sign bit in the JSON Output. I think we can compensate that quote easy |
|
Can you reconstruct this on your system or is it not so cold at your home? 😉 I'll log in a few measurements, just to understand that myself. |
|
My supply pipe is buried about 30m outside in the garden to pre-warm in winter and pre-cool in summer. Even at -14°C peak this night near Ingolstadt, I had +4°C in the supply chain. |
|
The issue - as I see - is:
What's happening now: |
|
As this will only happen with the outdoorTemperature I would recommend, that we just do:
We could (only for the outdoorTemperature key) pass it through the function: |
That explains why this has not yet been noticed. Unfortunately, my connection is not so protected. Near Regensburg it was -8°C and the Comfoclime measured -7°C. |
I'll take a closer look tomorrow and adapt my PR |
…n get_dashboard_data
- I've replaced the bitwise operations with Python-specific methods to make it more Pythonic. - Additionally, I deleted a few lines that did not contribute to solving this problem. - Revert: delete the property signed=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
after copilot review
|
As far as I can see, the "signed" argument when reading properties and telemetries is not passed to your new bytes_to_int function? That would treat every fetched value as signed, which is not correct. Or am I wrong? |
|
Also noticed and fixed this in this commit: ac2d83c |
Change default value of 'signed' parameter to False.
|
I think problems and doubts should now be resolved. |
|
In lines 149, 203 and 451 you call the conversion function but don't pass the argument signed. That's what I meant earlier. |
Sorry, I completely overlooked that. Now the parameters will pass to the methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._request_lock = asyncio.Lock() | ||
|
|
||
| @staticmethod | ||
| def bytes_to_signed_int(data: list, byte_count: int = None, signed:bool = True) -> int: |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma before signed parameter. Should be signed: bool with a space after the comma.
def bytes_to_signed_int(data: list, byte_count: int = None, signed: bool = True) -> int:| else: | ||
| raise ValueError(f"Nicht unterstützte Byte-Anzahl: {byte_count}") | ||
|
|
||
| value = self.bytes_to_signed_int(data, byte_count,signed) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma before signed parameter. Should be self.bytes_to_signed_int(data, byte_count, signed) with a space after the comma.
value = self.bytes_to_signed_int(data, byte_count, signed)| value = self.bytes_to_signed_int(data, byte_count,signed) | |
| value = self.bytes_to_signed_int(data, byte_count, signed) |
| if signed and value >= 0x8000: | ||
| value -= 0x10000 | ||
| if byte_count in (1, 2): | ||
| value = self.bytes_to_signed_int(data, byte_count,signed) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma before signed parameter. Should be self.bytes_to_signed_int(data, byte_count, signed) with a space after the comma.
value = self.bytes_to_signed_int(data, byte_count, signed)| if signed and raw_value < 0: | ||
| raw_value += 0x10000 | ||
| data = [raw_value & 0xFF, (raw_value >> 8) & 0xFF] | ||
| data = self.signed_int_to_bytes(raw_value, byte_count,signed) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma before signed parameter. Should be self.signed_int_to_bytes(raw_value, byte_count, signed) with a space after the comma.
data = self.signed_int_to_bytes(raw_value, byte_count, signed)| data = self.signed_int_to_bytes(raw_value, byte_count,signed) | |
| data = self.signed_int_to_bytes(raw_value, byte_count, signed) |
| """Convert raw bytes to a signed integer value. | ||
|
|
||
| Args: | ||
| data: List of bytes (integers 0-255) | ||
| byte_count: Number of bytes to read. If None calculate from data | ||
|
|
||
| Returns: | ||
| Signed integer value | ||
|
|
||
| Raises: | ||
| ValueError: If byte_count is not 1 or 2 | ||
| """ |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring is missing documentation for the signed parameter. The Args section should include:
Args:
data: List of bytes (integers 0-255)
byte_count: Number of bytes to read. If None calculate from data
signed: Whether to interpret the bytes as a signed integer (default: True)
I noticed that the outside temperature from the ComfoClime_api was unfortunately being received as a signed number. This meant that it was suddenly showing 6000°C outside. This pull request aims to fix that.