Repository navigation
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @KruseLuds
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Pull request overview
Adds richer Health Connect sleep session reporting by reusing a single SleepSessionRecord read per update cycle and deriving additional sensors for sleep start/end and per-stage durations, while keeping the existing sleep duration sensor for backward compatibility.
Changes:
- Added new Health Connect sensors for sleep start/end timestamps and standardized sleep-stage durations.
- Centralized sleep record processing via
updateSleepSensors()+analyzeSleepStages()to avoid duplicate Health Connect queries. - Added unit tests validating per-stage duration aggregation and timeline preservation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| common/src/main/res/values/strings.xml | Adds new sensor names/descriptions for sleep start/end and stage durations. |
| app/src/main/kotlin/io/homeassistant/companion/android/sensors/HealthConnectSensorManager.kt | Implements new sleep sensors and shared sleep-stage analysis + attribute building. |
| app/src/test/kotlin/io/homeassistant/companion/android/sensors/HealthConnectSensorManagerTest.kt | Adds tests for sleep-stage analysis aggregation and timeline preservation. |
| app/src/main/res/xml/changelog_master.xml | Documents the new Health Connect sleep sensors in the 2026.8.4 changelog. |
Suppressed comments (2)
app/src/main/kotlin/io/homeassistant/companion/android/sensors/HealthConnectSensorManager.kt:1135
stagesis passed as aList<Map<String, Any>>, but the attribute serialization inSensorManager.onSensorUpdatedtreats non-primitive lists asliststringand storesmap.toString()values. That means the sleep-stage timeline won’t actually arrive as structured data (type/start/end fields), which conflicts with the PR description and likely makes the attribute hard to consume reliably.
"deviceManufacturer" to sleepRecord.metadata.device?.manufacturer,
"deviceModel" to sleepRecord.metadata.device?.model,
"stages" to analysis.timeline,
)
app/src/main/kotlin/io/homeassistant/companion/android/sensors/HealthConnectSensorManager.kt:1133
buildSleepSessionAttributesincludestitle,notes, and record/client IDs plus device manufacturer/model. Unlike other Health Connect sensors in this file (which only expose time + source), this expands the data sent to Home Assistant and may unintentionally expose sensitive/free-text data (especiallynotes). Consider limiting attributes to what’s needed for consumers (e.g., start/end, source, and stage timeline) or making the extra metadata opt-in.
"title" to sleepRecord.title,
"notes" to sleepRecord.notes,
"sources" to sleepRecord.metadata.dataOrigin.packageName,
"recordId" to sleepRecord.metadata.id,
"lastModifiedTime" to sleepRecord.metadata.lastModifiedTime,
"clientRecordId" to sleepRecord.metadata.clientRecordId,
"clientRecordVersion" to sleepRecord.metadata.clientRecordVersion,
"recordingMethod" to sleepRecord.metadata.recordingMethod,
"deviceType" to sleepRecord.metadata.device?.type,
"deviceManufacturer" to sleepRecord.metadata.device?.manufacturer,
"deviceModel" to sleepRecord.metadata.device?.model,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Hi @KruseLuds
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
Bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
Done |
|
Sleep-stage timeline serialization Fixed. The nested list of maps has been replaced with parallel primitive lists for stage type, stage type ID, start time, and end time so the Companion App attribute serializer preserves the timeline structure. Unnecessary/sensitive sleep metadata fixed - Removed the additional title, notes, record/client IDs, device metadata, and other unnecessary sleep-session metadata. The attributes are now limited to the session times, source, and sleep-stage timeline data needed for this feature. |
|
All review feedback has been addressed and the latest changes are pushed. The targeted Health Connect tests and ktlint passed locally, and all GitHub CI checks are now green. Ready for maintainer review when someone has availability. Thanks! |
Do you mean with attributes? The core team really discourages the use of attributes. Looking at one of the best practices health integratios in core, Withings, it also exposes a lot of individual sensor for sleep stages etc not too dissimilar from what is proposed here: https://github.com/home-assistant/core/blob/83ca3988d515ddde893626a068d9f64d3e53a44a/homeassistant/components/withings/sensor.py#L329. As such, I think this is better than trying to send sleep data in attributes or events. |
…p-stages # Conflicts: # app/src/main/res/xml/changelog_master.xml
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hi @jpelgrom, Thanks for the clarification and the link to the documentation! I see that the five screenshot failures are all related to the updated changelog entry. I noticed the documentation also mentions a GitHub Actions workflow that maintainers can use to regenerate the reference screenshots. Would it be possible to use that workflow for this PR? I'd prefer to avoid setting up and running the full Android build environment locally just to regenerate the screenshots, as doing so would bring my Raspberry Pi to its knees. It's already running Home Assistant, and I don't have the resources to run such a demanding build alongside it, particularly since the compilation, lint, unit, and instrumentation tests are now passing. I'm sorry for any additional inconvenience this may cause, and I really appreciate your help and patience. If the maintainer workflow isn't an option for external PRs, please let me know what you'd recommend. Thanks again! |
|
The workflow can only be triggered for branches in this repository, not forks, sorry. We could update the screenshot test for you and push to your branch if that's the only remaining item for you. The branch is quite far behind though so please merge upstream first. Does it also mean that you haven't built the app to test your changes on a device or emulator? |
|
Hi @jpelgrom, Thank you again for offering to help with the screenshots. I really appreciate it! I've now merged the latest upstream I also tested the modified app on my actual Samsung phone, I downloaded the debug APK generated by GitHub Actions and connected it to a separate Home Assistant test instance running on my Windows PC. I'm happy to report that the Health Connect sleep-stage sensors are working! Light, Deep, REM, Awake, total sleep duration, and sleep start/end timestamps all populated successfully in Home Assistant. The measurements also closely matched the values shown in Samsung Health. I've attached a screenshot showing the actual sensor values in Home Assistant: 
The physical-device testing was performed with the APK generated before today's upstream merge, so the updated branch still needs its CI checks to complete. The latest workflow is now awaiting approval. If everything else looks good after the checks run, I'd be very grateful if you could help update the changelog reference screenshots as you offered. Thanks again for your time, patience, and help with this contribution! |
jpelgrom
changed the title
| ), | ||
| ChangelogEntry( | ||
| contentRes = commonR.string.changelog_entry_health_connect_sleep_stages, | ||
| platforms = setOf(ChangelogPlatform.APP, ChangelogPlatform.AUTOMOTIVE), |
There was a problem hiding this comment.
Does it make sense to have it for Automotive? You'll need to regenerate the changelog screenshots for this, it was not clear on your previous message you want us to generate it?
|
Hi @TimoPtr, Thank you for the detailed review. I've pushed the requested changes in commits The updates include:
The changes are ready for another look. The required checks are still pending, with a workflow awaiting maintainer approval. Oh - and with regards to your comment above, "...it was not clear on your previous message you want us to generate it?" - with regards to the screenshot tests that will still fail when it reaches that point - yes please! Thanks again! |
| SleepSessionRecord.STAGE_TYPE_AWAKE_IN_BED, | ||
| ) | ||
|
|
||
| assertEquals(expectedTypes, SleepSessionRecord.VALID_TYPES.toSet()) |
There was a problem hiding this comment.
Add a failure message that the usage needs to be checked because a type has been added. (removed would fail the build and I don't think it would happen).
There was a problem hiding this comment.
Was this addressed? There is a comment but not a failure message. It also only fails if something is removed...
There was a problem hiding this comment.
Adding this comment because I don't see easily how we could make this happen btw, with it being constants and not a sealed class and the list of all possible values restricted to the library.
|
Hi @TimoPtr, Thank you for the detailed review and helpful feedback. I've pushed the requested changes, including the ktlint formatting corrections, handling unknown sleep stage types without crashing, and a clearer failure message in the sleep-stage coverage test. I believe I've addressed all outstanding review comments. When you have a chance, I'd appreciate another look. Thanks again! |
| val sleepRecordDuration = calculateSleepDurationInMinutes(lastSleepRecord.stages) | ||
| onSensorUpdated( | ||
| val analysis = analyzeSleepStages(lastSleepRecord.stages) | ||
| val sessionAttributes = buildSleepSessionAttributes(lastSleepRecord, analysis) |
There was a problem hiding this comment.
This is only used for sleepDuration, so only calculate them if that sensor is enabled
| "endTime" to sleepRecord.endTime, | ||
| "sources" to sleepRecord.metadata.dataOrigin.packageName, | ||
| "stageTypes" to analysis.stageTypes, | ||
| "stageTypeIds" to analysis.stageTypeIds, |
There was a problem hiding this comment.
What is the point of including these IDs? They aren't documented anywhere and duplicate the human readable stages above.
| "stageTypeIds" to analysis.stageTypeIds, |
| <string name="changelog_entry_assistant_volume_sensor">Added an <b>assistant volume</b> sensor (Android 17+)</string> | ||
| <string name="changelog_entry_assistant_volume_command">Added a notification command to set the <b>assistant volume</b> (Android 17+)</string> | ||
| <string name="changelog_entry_health_connect"><b>Health Connect</b> sleep duration now ignores awake and out-of-bed time</string> | ||
| <string name="changelog_entry_health_connect_sleep_stages">Added <b>Health Connect</b> sleep start/end and individual sleep-stage sensors</string> |
There was a problem hiding this comment.
I would emphasize sleep, not just Health Connect
| <string name="changelog_entry_health_connect_sleep_stages">Added <b>Health Connect</b> sleep start/end and individual sleep-stage sensors</string> | |
| <string name="changelog_entry_health_connect_sleep_stages">Added more <b>Health Connect sleep sensors</b> for start/end time and individual sleep stages</string> |
|
Potential future conflict with #7619 (we'll monitor this, as well as the changelog screenshot update) |
|
I've addressed the review feedback, updated the changelog wording, and merged the latest upstream The changelog reference screenshots still need to be regenerated to reflect the updated Health Connect wording and the upstream file-picker entry. According to the Android screenshot-testing documentation, the CI screenshot-update workflow is restricted to maintainers and isn't available to external contributors using forks... @joostlek Would you or another maintainer (thank you @TimoPtr for your heklp with this before the wording changed!) be able to help regenerate the screenshots for this PR? Thank you! |
Summary
Adds richer Health Connect sleep session data to the Android Companion App by exposing the individual standardized sleep stages already available through
SleepSessionRecord.The existing Health Connect sleep duration sensor remains unchanged for backward compatibility. This change adds sensors for sleep start/end times and the duration of each standardized Health Connect sleep stage, including Light, Deep, REM, Awake, Awake in bed, Out of bed, Sleeping/unspecified, and Unknown.
The latest
SleepSessionRecordis read once per sensor update and its stages are analyzed once, avoiding duplicate Health Connect queries when multiple sleep sensors are enabled.The existing Sleep Duration sensor also preserves the ordered sleep-stage timeline as structured attributes containing each stage's type, start time, and end time, allowing Home Assistant consumers to use the original Health Connect stage data for dashboards and analysis.
The implementation is provider-neutral. It does not contain Samsung-, Fitbit-, or device-specific logic and does not attempt to merge separate
SleepSessionRecordentries.Missing stage data is kept unavailable/unknown rather than being converted to zero, because a provider not supplying a stage is semantically different from reporting zero duration.
Related feature request:
home-assistant/feature-requests#4080
Checklist
Select exactly one option that describes AI usage in this contribution:
Link to pull request in documentation repositories
User Documentation: home-assistant/companion.home-assistant#1407
Any other notes
Targeted
HealthConnectSensorManagerTest: 6/6 tests passed.:app:lintFullDebug: passed.ktlintCheck :build-logic:convention:ktlintCheck --continue: passed.The full
:app:testFullDebugUnitTestrun completed 1,530 tests with two unrelated failures inWearOnboardingNavigationTestandServerDiscoveryNavigationTest. Both failures were reproduced independently when those test classes were run in isolation.