Add sensor platform to Arcam FMJ#165271
Conversation
Add incoming video and audio diagnostic sensors for each zone (resolution, refresh rate, aspect ratio, colorspace, audio format, audio config, sample rate), disabled by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Hey there @elupus, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
PR Overview
This PR adds a new sensor platform to the Arcam FMJ integration. It introduces 8 diagnostic sensors per zone (16 total for zones 1 and 2), all disabled by default, exposing incoming video and audio stream properties: horizontal/vertical resolution, refresh rate, aspect ratio, colorspace, audio format, audio config, and audio sample rate.
Changes:
- New
sensor.pyimplementing diagnostic sensors for incoming video/audio parameters per zone, backed by theArcamFmjCoordinator - Updated
__init__.pyto addPlatform.SENSORto thePLATFORMSlist - Updated
strings.jsonwith entity names usingZone {zone}placeholders for all 8 sensor types - New
test_sensor.pywith tests covering sensor creation, value reading,Nonehandling, and coordinator lifecycle (connected/disconnected)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
homeassistant/components/arcam_fmj/sensor.py |
New sensor platform with 8 diagnostic sensor entity descriptions and the ArcamFmjSensorEntity class |
homeassistant/components/arcam_fmj/__init__.py |
Adds Platform.SENSOR to the PLATFORMS list to enable sensor setup |
homeassistant/components/arcam_fmj/strings.json |
Adds entity name translations for all 8 new sensor types using {zone} placeholder |
tests/components/arcam_fmj/test_sensor.py |
New test file covering sensor creation, value updates, None handling, and availability lifecycle |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Do those comments actually block these changes? Or can we just as easily clean them up once all actual functionality is in? |
- Replace getattr with direct None checks for video/audio parameters - Use SensorDeviceClass.ENUM for aspect ratio, colorspace, audio format and audio config sensors with snake_case options and translations - Fix audio format None handling to prevent IndexError - Use coordinator.state instead of cached self._state for native_value - Use entity_registry_enabled_by_default fixture in tests - Replace runtime_data access in tests with coordinator fixtures - Add test coverage for audio None values - Fix conftest mock defaults to use (None, None) for audio format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Looking at the comments I think this might be a "pls fix otherwise we might need to revert that PR" type of situation. I already gave one of the PRs a review, I think we should fix the issues raised before merging these ultimately |
…nsor # Conflicts: # tests/components/arcam_fmj/conftest.py
epenet
left a comment
There was a problem hiding this comment.
You need to fix merge conflicts, and open documentation PR
Documents the new diagnostic sensor entities added in home-assistant/core#165271, covering incoming video properties (resolution, refresh rate, aspect ratio, colorspace) and incoming audio properties (format, configuration, sample rate) per zone. https://claude.ai/code/session_01UnZxkNbBAS818QEdWU4qXh
Documents the new diagnostic sensor entities added in home-assistant/core#165271, covering incoming video properties (resolution, refresh rate, aspect ratio, colorspace) and incoming audio properties (format, configuration, sample rate) per zone. https://claude.ai/code/session_01UnZxkNbBAS818QEdWU4qXh
| @pytest.fixture(name="coordinator_1") | ||
| def coordinator_1_fixture( | ||
| hass: HomeAssistant, player_setup: str | ||
| ) -> ArcamFmjCoordinator: | ||
| """Get the coordinator for zone 1.""" | ||
| config_entry = hass.config_entries.async_entries("arcam_fmj")[0] | ||
| return config_entry.runtime_data.coordinators[1] | ||
|
|
||
|
|
||
| @pytest.fixture(name="coordinator_2") | ||
| def coordinator_2_fixture( | ||
| hass: HomeAssistant, player_setup: str | ||
| ) -> ArcamFmjCoordinator: | ||
| """Get the coordinator for zone 2.""" | ||
| config_entry = hass.config_entries.async_entries("arcam_fmj")[0] | ||
| return config_entry.runtime_data.coordinators[2] |
There was a problem hiding this comment.
Please remove these two fixtures.
You are using them to test internal and this is not allowed
|
|
||
| entity_description: ArcamFmjSensorEntityDescription | ||
| _attr_has_entity_name = True | ||
| _attr_entity_registry_enabled_default = False |
There was a problem hiding this comment.
It does not make sense to have these all disabled by default. the state update will have read them from device already so we gain little from disabling them.
There was a problem hiding this comment.
It could make sense to disable all that is part of zone 2 or higher.
There was a problem hiding this comment.
I agree, and that was my first choice. Someone - I thought you 😅 requested that these all be disabled, when I made my first pass at this like a year ago. Happy to enable them by default though
| } | ||
| }, | ||
| "incoming_audio_format": { | ||
| "name": "Zone {zone} incoming audio format", |
There was a problem hiding this comment.
We can drop these zone prefixes for entities now. I changed so the secondary zone ends up on a sub device instead.
Add incoming video and audio diagnostic sensors for each zone (resolution, refresh rate, aspect ratio, colorspace, audio format, audio config, sample rate), disabled by default.
This is a breakdown of #165098 per request, and a follow-up to #165232.
Proposed change
Add sensors that describe the properties of the incoming media.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: