Skip to content

Architecture violation refactor#82

Open
eman wants to merge 31 commits into
mainfrom
eval
Open

Architecture violation refactor#82
eman wants to merge 31 commits into
mainfrom
eval

Conversation

@eman
Copy link
Copy Markdown
Owner

@eman eman commented May 6, 2026

No description provided.

eman and others added 11 commits May 5, 2026 23:52
- Add MQTT_PROTOCOL_VERSION constant to config.py; use in control.py
- Fix set_vacation_days to delegate to set_dhw_mode
- Consolidate duplicate NavienBaseModel into shared _base.py
- Fix per-device state tracking: _previous_status is now dict[str, DeviceStatus] keyed by MAC
- Remove set_unit_system() side-effects from constructors; store as instance variable
- Update auth.py, api_client.py, mqtt/client.py to use UnitSystemType
- Store raw device values as *_raw int fields with original JSON aliases
- Add computed properties for all temperature, flow rate, and volume fields
- Conversion happens lazily at access time via get_unit_system() / temperature_type
- Remove WrapValidator functions from converters.py
- Remove set_unit_system workaround from mqtt/subscriptions.py _make_handler
- Update get_field_unit() to look up metadata from _raw fields

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Phase 3 (Topic consolidation):
- Add response_ack_topic() to MqttTopicBuilder for control command acks
- Document all four topic schema patterns in MqttTopicBuilder docstring
- Update _build_command in control.py to use MqttTopicBuilder
- Fix reservations.py to use MqttTopicBuilder.response_topic (remove hardcoded string)
- Fix cli/handlers.py to use MqttTopicBuilder.response_topic (remove hardcoded string)
- Remove set_unit_system workaround from reservations.py fetch_reservations

Phase 4 (State tracker extraction):
- Create mqtt/state_tracker.py with DeviceStateTracker class
- Move _detect_state_changes logic out of MqttSubscriptionManager
- Wire DeviceStateTracker into MqttSubscriptionManager via _state_tracker
- clean up unused get_unit_system import from subscriptions.py
All event emitters now wrap args in typed dataclass instances from
mqtt_events.py before calling emit(). Handlers receive a single event
object with named fields instead of multiple positional args.

Affected emitters:
- state_tracker.py: temperature_changed, mode_changed, power_changed,
  heating_started/stopped, error_detected/cleared
- subscriptions.py: status_received, feature_received
- mqtt/client.py: connection_interrupted, connection_resumed

Updated handlers:
- examples/advanced/reconnection_demo.py
- examples/advanced/mqtt_diagnostics.py
- examples/intermediate/command_queue.py
- examples/intermediate/event_driven_control.py

Updated docs in mqtt_events.py and events.py to reflect new signatures

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rol methods

Phase 5: Typed event payloads (already applied in state_tracker.py / client.py)
  - ConnectionInterruptedEvent, ConnectionResumedEvent wrapped on emit

Phase 6: New protocol models in models.py
  - WeeklyReservationEntry + WeeklyReservationSchedule (RESERVATION_WEEKLY)
  - RecirculationScheduleEntry + RecirculationSchedule (RECIR_RESERVATION)
  - OtaCommitPayload (OTA_COMMIT - commitOta structure)
  - Export all new models from __init__.py

Phase 7: Implement 9 missing control methods in mqtt/control.py
  - update_weekly_reservation(device, WeeklyReservationSchedule)
  - check_firmware_update(device)
  - commit_firmware_update(device, OtaCommitPayload)
  - reconnect_wifi(device)
  - reset_wifi(device)
  - set_freeze_protection_temperature(device, temperature)
  - run_smart_diagnostic(device)
  - enable_intelligent_scheduling(device)
  - disable_intelligent_scheduling(device)
  - configure_recirculation_schedule: typed RecirculationSchedule (was dict)
…on responses

- Add from_dict() to ReservationSchedule, WeeklyReservationSchedule, RecirculationSchedule
- Add subscribe_reservation_response() to MqttSubscriptionManager
- Add subscribe_weekly_reservation_response() to MqttSubscriptionManager
- Add subscribe_recirculation_schedule_response() to MqttSubscriptionManager
- Proxy all three new methods on NavienMqttClient
- Refactor fetch_reservations() to use typed subscribe_reservation_response()
  instead of raw mqtt.subscribe() with manual topic/message filtering
- Update tests to match new typed callback interface
Add proxy methods for all 31 public MqttDeviceController methods directly
on NavienMqttClient, so users don't need to use mqtt.control.method():
  - request_device_status, request_device_info
  - set_power, set_dhw_mode, set_dhw_temperature, set_vacation_days
  - enable/disable_anti_legionella
  - update_reservations, request_reservations
  - update_weekly_reservation, configure_reservation_water_program
  - configure_recirculation_schedule, set_recirculation_mode
  - trigger_recirculation_hot_button
  - configure_tou_schedule, request_tou_settings, set_tou_enabled
  - request_energy_usage, signal_app_connection
  - enable/disable_demand_response, reset_air_filter
  - check_firmware_update, commit_firmware_update
  - reconnect_wifi, reset_wifi, set_freeze_protection_temperature
  - run_smart_diagnostic
  - enable/disable_intelligent_scheduling

Update reservations.py to use mqtt.request_reservations() and
mqtt.update_reservations() directly instead of mqtt.control.*
Update tests to match new mock interface.
Split 1966-line models.py monolith into 9 focused submodules:
- models/_converters.py: temperature helper functions
- models/device.py: DeviceInfo, Location, Device, FirmwareInfo
- models/tou.py: TOUSchedule, ConvertedTOUPlan, TOUInfo
- models/schedule.py: all reservation and schedule models + OtaCommitPayload
- models/status.py: DeviceStatus
- models/feature.py: DeviceFeature
- models/mqtt_models.py: MqttRequest, MqttCommand
- models/energy.py: EnergyUsage* models
- models/__init__.py: re-exports everything (no API change)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@eman
Copy link
Copy Markdown
Owner Author

eman commented May 7, 2026

@copilot what happened to the review that I requested yesterday?

Copy link
Copy Markdown
Contributor

Copilot AI commented May 7, 2026

@copilot what happened to the review that I requested yesterday?

I missed your request yesterday. I’m on it now—the current branch state is at b00236f, and I’ve resumed handling the review thread from here.

Copilot finished work on behalf of eman May 7, 2026 21:30
Comment thread src/nwp500/reservations.py Dismissed
eman and others added 11 commits May 7, 2026 16:15
- Fix syntax errors and truncated code in src/nwp500/models/schedule.py
- Add 'from __future__ import annotations' to all source files to fix runtime NameErrors
- Resolve all ruff linting and formatting issues (E501, F401, etc.)
- Fix pyright type-checking errors in AWS credential handling and model methods
- Update tests/test_reservations.py to match high-level MQTT client API
- Restore AuthTokens.to_dict() for compatibility
…cedence

- Add get_response_data() helper to mqtt/utils.py that checks nested
  response dict before top-level, and primary key before alt key:
  res[key] → res[alt_key] → message[key] → message[alt_key]
- Use helper in _make_handler() fixing the fallback order (previously
  message[key] was checked before res[alt_key])
- Use same helper in CLI raw_cb path so 'st'/'did' alt keys are
  supported there too, consistent with the typed handler path
- Fix missing return None in EnergyUsageResponse.get_month_data
- Fix mypy no-any-return in MqttConnection.unsubscribe/publish
Replace or-chain with explicit key-presence checks using a sentinel
object, so falsy values (0, False, {}) are returned correctly and do
not fall through to a lower-precedence candidate.

Also add unit tests for the raw=True CLI path with 'st'/'did' alt keys
and the standard 'status'/'feature' keys.
fix(mqtt): support both 'st/did' and 'status/feature' keys
Adds typed TOU schedule subscription to complete API symmetry with the
other scheduling typed subscriptions:
- subscribe_reservation_response()    → ReservationSchedule
- subscribe_weekly_reservation_response() → WeeklyReservationSchedule
- subscribe_recirculation_schedule_response() → RecirculationSchedule
- subscribe_tou_response() (new)      → TOUInfo

The TOUInfo model and request_tou_settings() control method already
existed; only the typed subscription callback was missing.

Changes:
- MqttSubscriptionManager.subscribe_tou_response()
- MqttSubscriptionManager.unsubscribe_tou_response()
- NavienMqttClient.subscribe_tou_response() (proxy)
- NavienMqttClient.unsubscribe_tou_response() (proxy)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace incorrect TOUInfo model (API-level) with new TOUReservationSchedule
that matches the actual tou/rd MQTT payload structure (reservationUse +
reservation period list).

Add TOUPeriod model for individual TOU pricing periods with fields:
season, week, startHour/Minute, endHour/Minute, priceMin, priceMax,
decimalPoint, plus computed properties for formatted times and decoded prices.

Export TOUPeriod and TOUReservationSchedule from models and top-level package.
- Rename start_minute/end_minute -> start_min/end_min in TOUPeriod to
  match existing scheduling model conventions (startMin/endMin aliases
  kept for MQTT payload compatibility)
- Fix reservationUse docstring: 0=disabled, 2=enabled (not 1=disabled)
- Add subscribe_tou_response()/unsubscribe_tou_response() to
  docs/python_api/mqtt_client.rst
Inject MAC address into DeviceStatus and DeviceFeature models and propagate
device_mac through all device-specific MQTT events. This ensures that
consumers can distinguish between multiple devices in event-driven and
callback-based monitoring flows without manual routing or closures.

- Add mac_address field to DeviceStatus and DeviceFeature models
- Add device_mac to all MQTT event dataclasses
- Update MqttSubscriptionManager to populate model identity on receipt
- Update DeviceStateTracker to include identity in emitted change events
- Add comprehensive multi-device verification tests
eman and others added 8 commits May 7, 2026 22:50
Addressing E501 (line too long), F401 (unused imports), and I001 (import sorting) in subscriptions.py and test_multi_device.py to fix CI failures.
- Add missing click and rich dependencies to docs/requirements.txt
- Fix typo in POWER_CHANGED docstring in mqtt_events.py
- Fix nested list formatting in CHANGELOG.rst
- Fix malformed ASCII table in energy_monitoring.rst
- Reorganized docs/ into tutorials, how-to, reference, and explanation quadrants.
- Updated README.rst and docs/index.rst to reflect the new structure.
- Systematic repair of internal :doc: and :ref: links.
- Added a new Architecture explanation document.
- Updated conf.py for the new API documentation location.
The _on_connection_resumed_internal callback was missing the 'connection'
parameter required by the AWS IoT SDK callback signature. The AWS SDK calls
on_connection_resumed(connection, return_code, session_present, **kwargs),
but the handler only accepted (return_code, session_present).

This caused the callback to fail silently (exception swallowed by the AWS
CRT C layer) when the SDK auto-reconnected after AWS_ERROR_MQTT_UNEXPECTED_HANGUP.
As a result:
- self._connected was never set back to True
- connection_resumed event was never emitted
- The reconnection handler loop kept running indefinitely
- Sensors became permanently unavailable until manual restart

Fix: add 'connection' as the first parameter and '**kwargs' for
forward-compatibility, matching the interrupted callback's signature.
Also update the type annotation in MqttConnection to match.

Fixes #85
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@eman
Copy link
Copy Markdown
Owner Author

eman commented May 12, 2026

Fix: MQTT reconnection after unexpected AWS hangup (#85)

Added two commits that fix the reconnection regression reported in issue #85:

Root cause: _on_connection_resumed_internal had the wrong callback signature. The AWS IoT SDK calls on_connection_resumed(connection, return_code, session_present, **kwargs), but the handler only declared (return_code, session_present) — missing connection as the first positional argument.

This caused the callback to raise a TypeError silently (swallowed by the AWS CRT C layer), so:

  • self._connected was never restored to True after the SDK auto-reconnected
  • connection_resumed event was never emitted
  • The manual reconnection loop in _reconnect_with_backoff spun indefinitely
  • Sensors became permanently unavailable until manual restart

Fix: Added connection: mqtt.Connection as the first parameter and **kwargs: Any for forward-compatibility, consistent with the on_connection_interrupted callback's existing correct signature. Updated the MqttConnection type annotation to match.

Commits: ac440a4 (fix), 37e8e5f (changelog)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants