refactor: extract decoders into separate modules #292
Conversation
WalkthroughThe decoder module architecture is refactored to separate individual decoder implementations from the main entry point. Decoder classes (UrlDecoder, Df3Decoder, Df5Decoder, Df6Decoder, DfE1Decoder, HistoryDecoder, AirHistoryDecoder) are extracted into standalone modules within a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
ruuvitag_sensor/decoders/air_history_decoder.py (1)
54-61: Temperature decoding logic can be simplified.The condition on line 59 is redundant since line 57 already handles the
0x8000sentinel case. After line 58 returns, any remaining value>= 0x8000represents a negative temperature that needs two's complement conversion. Consider usingsigned=Trueinfrom_bytesdirectly to simplify:def _get_temperature(self, record_data: bytearray) -> float | None: """Extract temperature from record (bytes 5-6, int16_t BE, x 200).""" - temp_raw = int.from_bytes(record_data[5:7], byteorder="big", signed=False) - if temp_raw == 0x8000: + temp_raw = int.from_bytes(record_data[5:7], byteorder="big", signed=True) + if temp_raw == -32768: # 0x8000 as signed int16 return None - if temp_raw >= 0x8000: - temp_raw = temp_raw - 0x10000 return round(temp_raw / 200.0, 2)ruuvitag_sensor/decoders/url_decoder.py (1)
46-46: Missing type annotation forencodedparameter.The
encodedparameter lacks a type annotation. Based on the usage (length check and slicing), it should be typed asstr.- def decode_data(self, encoded) -> SensorDataUrl | None: + def decode_data(self, encoded: str) -> SensorDataUrl | None:ruuvitag_sensor/decoders/dfe1_decoder.py (2)
92-98: Redundantbytes()call on already-bytes value.
data[11]fromstruct.unpack(..., "3s", ...)is already abytesobject. Thebytes()wrapper is unnecessary but harmless.def _get_luminosity_lux(self, data: ByteData) -> float | None: """Return Luminosity Lux""" - lumi_bytes = bytes(data[11]) + lumi_bytes = data[11] if lumi_bytes == b"\xff\xff\xff": return None lumi_val = int.from_bytes(lumi_bytes, byteorder="big") return round(lumi_val * 0.01, 2)
100-111: Same redundantbytes()pattern in other methods.Similar to
_get_luminosity_lux, thebytes()wrappers on lines 101 and 111 are unnecessary sincedata[13]anddata[16]are alreadybytesobjects from the struct unpack.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ruuvitag_sensor/decoder.py(2 hunks)ruuvitag_sensor/decoders/__init__.py(1 hunks)ruuvitag_sensor/decoders/air_history_decoder.py(1 hunks)ruuvitag_sensor/decoders/df3_decoder.py(1 hunks)ruuvitag_sensor/decoders/df5_decoder.py(1 hunks)ruuvitag_sensor/decoders/df6_decoder.py(1 hunks)ruuvitag_sensor/decoders/dfe1_decoder.py(1 hunks)ruuvitag_sensor/decoders/history_decoder.py(1 hunks)ruuvitag_sensor/decoders/url_decoder.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
ruuvitag_sensor/decoders/air_history_decoder.py (1)
ruuvitag_sensor/ruuvi_types.py (1)
SensorAirHistoryData(71-83)
ruuvitag_sensor/decoders/url_decoder.py (2)
ruuvitag_sensor/ruuvi_types.py (1)
SensorDataUrl(10-14)ruuvitag_sensor/decoders/df3_decoder.py (3)
_get_temperature(19-35)_get_humidity(37-39)_get_pressure(41-43)
ruuvitag_sensor/decoders/df5_decoder.py (1)
ruuvitag_sensor/ruuvi_types.py (1)
SensorData5(28-41)
ruuvitag_sensor/decoders/dfe1_decoder.py (1)
ruuvitag_sensor/ruuvi_types.py (1)
SensorDataE1(58-61)
ruuvitag_sensor/decoders/history_decoder.py (1)
ruuvitag_sensor/ruuvi_types.py (1)
SensorHistoryData(64-68)
ruuvitag_sensor/decoder.py (7)
ruuvitag_sensor/decoders/air_history_decoder.py (1)
AirHistoryDecoder(10-179)ruuvitag_sensor/decoders/df3_decoder.py (1)
Df3Decoder(12-76)ruuvitag_sensor/decoders/df5_decoder.py (1)
Df5Decoder(12-121)ruuvitag_sensor/decoders/df6_decoder.py (1)
Df6Decoder(12-154)ruuvitag_sensor/decoders/dfe1_decoder.py (1)
DfE1Decoder(11-141)ruuvitag_sensor/decoders/history_decoder.py (1)
HistoryDecoder(10-142)ruuvitag_sensor/decoders/url_decoder.py (1)
UrlDecoder(11-70)
ruuvitag_sensor/decoders/df6_decoder.py (1)
ruuvitag_sensor/ruuvi_types.py (1)
SensorData6(44-55)
🔇 Additional comments (17)
ruuvitag_sensor/decoders/air_history_decoder.py (1)
134-179: Well-structured decode_data implementation.The main decode loop has proper validation, clear error handling, and appropriate debug logging. The use of the walrus operator on line 176 for conditional appending is clean and idiomatic Python.
ruuvitag_sensor/decoders/url_decoder.py (1)
29-45: LGTM!The temperature, humidity, and pressure extraction methods correctly implement the DF2/DF4 protocol specification with proper sign handling for temperature.
ruuvitag_sensor/decoders/df6_decoder.py (3)
83-99: Luminosity decoding implementation looks correct.The logarithmic decoding logic properly handles the edge cases (255 for invalid, 0 for zero) and correctly reverses the logarithmic encoding.
57-81: VOC and NOx 9-bit extraction is well-documented.The bit manipulation for extracting 9-bit values from the combination of data bytes and flags is correctly implemented with clear comments explaining the bit layout.
122-136: The struct format is correct. The calculation in the original comment contains an arithmetic error. The format string">BhHHHHBBBBBBBBB"unpacks exactly 20 bytes: 1 (data_format) + 2 (temperature) + 2 (humidity) + 2 (pressure) + 2 (pm_2_5) + 2 (co2) + 1 (voc_low) + 1 (nox_low) + 1 (luminosity) + 1 (reserved) + 1 (measurement_sequence) + 1 (flags) + 1 (mac[0]) + 1 (mac[1]) + 1 (mac[2]) = 20 bytes, which matchesdata[:40]exactly. The 3-byte MAC at indices 12-14 is properly included in the format string. No changes are needed.ruuvitag_sensor/decoders/df3_decoder.py (2)
19-35: Excellent documentation of temperature encoding quirks.The detailed comment explaining the sign-magnitude encoding (MSB as sign, lower 7 bits as value) rather than two's complement is valuable for maintainability. The implementation correctly handles this non-standard encoding.
53-76: LGTM!The decode_data implementation correctly unpacks the 14-byte DF3 payload and computes all sensor values including the acceleration magnitude. Error handling with exception logging is appropriate.
ruuvitag_sensor/decoders/__init__.py (1)
1-30: Clean package organization with explicit exports.The
__init__.pyproperly consolidates all decoder classes with a clear docstring documenting each format's purpose and deprecation status. The explicit__all__list ensures predictable import behavior.ruuvitag_sensor/decoders/history_decoder.py (1)
10-29: Well-documented class with clear protocol specification.The docstring provides a comprehensive overview of the data format and special cases. The structure is clean and maintainable.
ruuvitag_sensor/decoder.py (3)
1-23: Clean backward-compatibility shim with helpful documentation.Good approach to maintain backward compatibility while migrating to a cleaner module structure. The comments guide users to the new import paths.
43-83: LGTM!The
get_decoderfunction is well-structured with appropriate deprecation warnings for obsolete formats and clear error handling for unknown formats.
86-103: LGTM!The
parse_macfunction correctly formats MAC addresses for Data Format 5 while passing through other formats unchanged.ruuvitag_sensor/decoders/df5_decoder.py (3)
12-17: LGTM!Clear class documentation with protocol specification reference.
54-60: LGTM!The invalid value check correctly uses the 11-bit marker value
0b11111111111(2047) for the battery voltage field.
86-121: Solid decode implementation with appropriate error handling.The
decode_datamethod correctly unpacks the binary payload and handles exceptions gracefully. The inline comment explaining thetype: ignorecomments is helpful.ruuvitag_sensor/decoders/dfe1_decoder.py (2)
11-16: LGTM!Clear class documentation with protocol specification reference.
113-141: LGTM!The
decode_datamethod properly unpacks the E1 format payload and aggregates all sensor values with appropriate error handling.
| acc_x, acc_y, acc_z = self._get_acceleration(byte_data) | ||
| acc = math.sqrt(acc_x * acc_x + acc_y * acc_y + acc_z * acc_z) if acc_x and acc_y and acc_z else None |
There was a problem hiding this comment.
Truthy check may incorrectly skip calculation when acceleration is zero.
The condition if acc_x and acc_y and acc_z uses truthy evaluation. If any component is 0, the condition fails and acc becomes None even though 0 is a valid acceleration value.
acc_x, acc_y, acc_z = self._get_acceleration(byte_data)
- acc = math.sqrt(acc_x * acc_x + acc_y * acc_y + acc_z * acc_z) if acc_x and acc_y and acc_z else None
+ acc = math.sqrt(acc_x * acc_x + acc_y * acc_y + acc_z * acc_z) if acc_x is not None and acc_y is not None and acc_z is not None else None🤖 Prompt for AI Agents
In ruuvitag_sensor/decoders/df5_decoder.py around lines 97-98, the current
truthy check "if acc_x and acc_y and acc_z" will skip computing magnitude when
any component is zero; update the condition to explicitly check for None (e.g.,
"if acc_x is not None and acc_y is not None and acc_z is not None") so zero
values are allowed, then compute acc = math.sqrt(acc_x*acc_x + acc_y*acc_y +
acc_z*acc_z) otherwise leave acc as None.
| def _get_voc_index(self, data: ByteData) -> int | None: | ||
| """Return VOC index (unitless, 9-bit)""" | ||
| # VOC: bits [8:1] are in data[6], bit [0] (LSB) is FLAGS bit 6 | ||
| # As per spec: "9 bit unsigned, least significant bit in Flags byte" | ||
| voc_high_bits = data[9] # bits [8:1] | ||
| voc_lsb = (data[14] >> 6) & 0x01 # bit [0] | ||
| voc = (voc_high_bits << 1) | voc_lsb | ||
|
|
||
| if voc == 511: # 0x1FF | ||
| return None | ||
|
|
||
| return voc | ||
|
|
||
| def _get_nox_index(self, data: ByteData) -> int | None: | ||
| """Return NOx index (unitless, 9-bit)""" | ||
| # NOx: bits [8:1] are in data[7], bit [0] (LSB) is FLAGS bit 7 | ||
| # As per spec: "9 bit unsigned, least significant bit in Flags byte" | ||
| nox_high_bits = data[10] # bits [8:1] | ||
| nox_lsb = (data[14] >> 7) & 0x01 # bit [0] | ||
| nox = (nox_high_bits << 1) | nox_lsb | ||
|
|
||
| if nox == 511: # 0x1FF | ||
| return None | ||
|
|
||
| return nox |
There was a problem hiding this comment.
Misleading comments reference wrong data indices.
The comments on lines 68 and 81 say "bits [8:1] are in data[6]" and "data[7]" respectively, but the code correctly uses data[9] for VOC and data[10] for NOx. Update the comments to match the actual indices.
def _get_voc_index(self, data: ByteData) -> int | None:
"""Return VOC index (unitless, 9-bit)"""
- # VOC: bits [8:1] are in data[6], bit [0] (LSB) is FLAGS bit 6
+ # VOC: bits [8:1] are in data[9], bit [0] (LSB) is FLAGS bit 6
# As per spec: "9 bit unsigned, least significant bit in Flags byte"
voc_high_bits = data[9] # bits [8:1]
...
def _get_nox_index(self, data: ByteData) -> int | None:
"""Return NOx index (unitless, 9-bit)"""
- # NOx: bits [8:1] are in data[7], bit [0] (LSB) is FLAGS bit 7
+ # NOx: bits [8:1] are in data[10], bit [0] (LSB) is FLAGS bit 7
# As per spec: "9 bit unsigned, least significant bit in Flags byte"
nox_high_bits = data[10] # bits [8:1]🤖 Prompt for AI Agents
In ruuvitag_sensor/decoders/dfe1_decoder.py around lines 66 to 90 the inline
comments incorrectly state that the 9-bit high bits for VOC and NOx are in
data[6] and data[7]; update those comments to match the actual code which reads
data[9] for VOC and data[10] for NOx (keep the rest of the comment, e.g., that
the LSB comes from FLAGS byte at data[14], unchanged) so the comments accurately
reflect the indices used.
| # Temperature is in 0.01°C units, little-endian | ||
| temp_bytes = bytes.fromhex("".join(data[9:11])) | ||
| temp_raw = int.from_bytes(temp_bytes, "big") | ||
| return round(temp_raw * 0.01, 2) |
There was a problem hiding this comment.
Docstring says "little-endian" but code uses big-endian.
The comment on line 51 states "little-endian" but int.from_bytes(temp_bytes, "big") on line 53 uses big-endian. The same inconsistency appears in _get_humidity (line 60-62) and _get_pressure (line 69-71). Please verify against the protocol spec and fix either the comment or the byte order.
🤖 Prompt for AI Agents
In ruuvitag_sensor/decoders/history_decoder.py around lines 51-54, the comment
states the temperature value is little-endian but the code uses
int.from_bytes(..., "big"); change the byte-order argument to "little" for
temperature and the same three places for humidity (lines ~60-62) and pressure
(lines ~69-71) so the code matches the comment/protocol; keep the bytes.fromhex
and scaling/rounding logic intact and run unit tests or compare against known
sample packets to verify values.
| case _: | ||
| log.info("Invalid packet type: %d - %s", packet_type, data) | ||
| return None |
There was a problem hiding this comment.
TypeError: %d format specifier used with string value.
packet_type is a string (e.g., "30", "31"), but %d expects an integer. This will raise a TypeError when an unknown packet type is logged.
case _:
- log.info("Invalid packet type: %d - %s", packet_type, data)
+ log.info("Invalid packet type: %s - %s", packet_type, data)
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case _: | |
| log.info("Invalid packet type: %d - %s", packet_type, data) | |
| return None | |
| case _: | |
| log.info("Invalid packet type: %s - %s", packet_type, data) | |
| return None |
🤖 Prompt for AI Agents
In ruuvitag_sensor/decoders/history_decoder.py around lines 136 to 138, the log
call uses the %d specifier but packet_type is a string which causes a TypeError;
update the log to use a string formatter (e.g. change "%d" to "%s") or
explicitly cast packet_type to int only if you first validate it is numeric, so
the logging will not raise on unknown packet types and still include the
packet_type and data.
1 participant
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.