Conversation
BREAKING CHANGES: - Migrated command codes to DeviceControl enum - Renamed TemperatureUnit to TemperatureType - Updated capability flags to use CapabilityFlag type Added: - src/nwp500/enums.py with 12+ enumerations for device protocol - ErrorCode enum with complete error mappings - DeviceControl enum for all MQTT commands - Status enums: OnOffFlag, Operation, DhwOperationSetting, etc. - Human-readable text mappings for all enums - docs/enumerations.rst comprehensive reference - examples/error_code_demo.py showing enum usage Changed: - CLI output uses enum-based text mappings - Documentation updated across guides and protocol docs - Examples updated to use new enums - Command constants: ANTI_LEGIONELLA_ENABLE→ON, TOU_ENABLE→ON, etc. Fixed: - Temperature conversion test now uses correct HalfCelsiusToF formula
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive enumerations module that centralizes all device protocol enums into type-safe IntEnum classes. The key improvement is replacing magic numbers and scattered enum definitions with a unified, well-documented enums module that provides type safety, human-readable text mappings, and consistent naming conventions.
Key Changes:
- Created comprehensive enums module with 12+ enumerations for device protocol
- Migrated command constants to consistent naming (e.g., ANTI_LEGIONELLA_ENABLE → ANTI_LEGIONELLA_ON)
- Fixed temperature conversion test to use correct HalfCelsiusToF formula
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/nwp500/enums.py | New comprehensive enumerations module with status values, commands, error codes, and text mappings |
| src/nwp500/models.py | Updated to use enums from new module, added enum serialization to model_dump(), changed TemperatureUnit to TemperatureType |
| src/nwp500/constants.py | Renamed command constants for consistency (ENABLE→ON, DISABLE→OFF, SETTINGS→RESERVATION) |
| src/nwp500/init.py | Exports new enums for public API |
| src/nwp500/mqtt_device_control.py | Updated to use renamed command constants |
| src/nwp500/cli/output_formatters.py | Simplified enum serialization (now handled by model_dump()) |
| src/nwp500/cli/commands.py | Refactored to use format_json_output helper |
| tests/test_models.py | Fixed temperature conversion test formula |
| examples/*.py | Updated examples to use new enums |
| docs/*.rst | Updated documentation to reference new enums and clarify TOU override behavior |
Comments suppressed due to low confidence (1)
src/nwp500/enums.py:1
- The class name
OperationModeis introduced in enums.py but the PR description and documentation refer to it asDhwOperationSetting. The models.py file importsDhwOperationSettingfrom enums.py, but enums.py definesOperationMode. Either the import in models.py should useOperationMode, or the class in enums.py should be renamed toDhwOperationSettingfor consistency.
"""Enumerations for Navien device protocol.
- Replace OperationMode with DhwOperationSetting (user-configured mode) - Replace OperationMode with CurrentOperationMode (real-time state) - Update text mapping examples to use correct constants - Add both enums to enumerations.rst with clear explanations - Update models.rst type annotations and examples
The use_enum_values=False parameter controls deserialization behavior (keeps enums as enum objects during validation), not serialization. Actual serialization to names is handled by the custom model_dump() method.
DhwOperationSetting does not have a HYBRID member. The hybrid modes are: - ENERGY_SAVER (value 3) - Hybrid: Efficiency mode - HIGH_DEMAND (value 4) - Hybrid: Boost mode
- Changed 'Hybrid mode' to 'Energy Saver mode' for HYBRID_EFFICIENCY_MODE - Added missing handler for HYBRID_BOOST_MODE (High Demand mode) - Ensures mode names match actual enum values and user-facing terminology
Changed two comparisons from checking OnOffFlag.OFF to OnOffFlag.ON: - smart_diagnostic_use: OFF (1) = not supported, ON (2) = supported - mixing_value_use: OFF (1) = not supported, ON (2) = supported The previous logic showed 'Yes' when the flag was OFF, which was incorrect.
eman
commented
Dec 17, 2025
Contributor
Reverted magic number 5 back to DhwOperationSetting.VACATION.value. Added missing import from enums module. Updated docstring to avoid magic number reference.
Fixed magic number usage in: - examples/anti_legionella_example.py: 2 -> OnOffFlag.ON - examples/combined_callbacks.py: 2 -> OnOffFlag.ON - src/nwp500/mqtt_client.py: Updated docstring All enum values now use proper enum references for type safety.
Moved CommandCode from constants.py to enums.py for better organization: - All enumerations now in a single module - constants.py now only contains firmware metadata - Backward compatibility maintained via re-export in constants.py - Updated imports in mqtt_device_control.py to use enums module - Updated __init__.py to export CommandCode from enums This consolidates all protocol enumerations in one place.
Per project policy, no backward compatibility needed. - Removed re-export from constants.py - Updated constants.py to note new import location - Added breaking change to CHANGELOG with migration example Users must update: from nwp500.constants -> from nwp500.enums
Updated import to use nwp500.enums instead of nwp500.constants
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/nwp500/enums.py:1
- The class name
DeviceControlis inconsistent with the PR description and documentation which refer toCommandCode. The CHANGELOG and imports show this enum should be namedCommandCodeto match the original constant class it replaces. This inconsistency will cause confusion as developers expect to importCommandCodebut the enum is actually namedDeviceControl.
"""Enumerations for Navien device protocol.
The enum is named CommandCode, not DeviceControl. - Updated docs/enumerations.rst: DeviceControl -> CommandCode - Updated docs/python_api/models.rst: DeviceControl -> CommandCode - Updated CHANGELOG.rst: All DeviceControl references -> CommandCode This matches what developers actually import and use.
Prevents potential infinite recursion on circular references: - Added 'visited' parameter to track processed objects - Returns early if a circular reference is detected - Uses set of object ids to identify already-processed items - Fixes potential stack overflow with circular data structures This is defensive programming for safety even though Pydantic models should not produce circular references in normal use.
Signal handlers run in different execution contexts and can cause race conditions when modifying shared state. Fixed by: - Replaced self.running = False with asyncio.Event - Signal handler schedules shutdown via asyncio.create_task - Loop conditions check shutdown_event.is_set() instead of self.running - Eliminates race conditions between signal handler and main loop This is a safer pattern for async code with signal handlers.
Format strings with IntEnum values should use .value explicitly
rather than relying on implicit conversion. This improves:
- Clarity of intent (explicitly accessing the value)
- Type checking compliance
- Reduces error-prone implicit conversions
Changed E{error_code:03d} to E{error_code.value:03d} in two places.
Updated two remaining references to self.running attribute that was removed in favor of asyncio.Event: - Line 253: while condition now uses 'not self.shutdown_event.is_set()' - Line 262: cleanup now calls 'self.shutdown_event.set()' Prevents AttributeError at runtime in the main example loop.
Removed result_val temporary variable and return directly from type(data)(converted) for cleaner, more concise code without sacrificing readability.
Changed from incorrect 'CAS\_' with escaped underscore to proper inline literal syntax using backticks: 'CAS_' renders correctly in reStructuredText without escape sequences.
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.
Summary
This PR introduces a comprehensive enumerations module (src/nwp500/enums.py) that provides type-safe enums for device control commands, status values, and error codes.
Breaking Changes
Command Code Constants: Migrated to DeviceControl enum with consistent naming
Model Types:
What's Added
12+ Enumerations covering device protocol:
Human-readable text mappings for all enums
Documentation: docs/enumerations.rst with complete reference
Examples: examples/error_code_demo.py demonstrating enum usage
What's Changed
What's Fixed
Testing
Documentation