New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Sonos alarms and favorites updating #55529
Refactor Sonos alarms and favorites updating #55529
Conversation
Hey there @cgtobi, mind taking a look at this pull request as it has been labeled with an integration ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Clean and nice code.
Seems to work as well
self.hass, SONOS_CREATE_ALARM, speaker, [alarm.alarm_id] | ||
) | ||
updated = await self.hass.async_add_executor_job( | ||
self.update_cache, soco, update_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this no longer throw (OSError, SoCoException)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's still possible. Will catch during the update.
for alarm_id, alarm in self.alarms.alarms.items(): | ||
if alarm_id in self.created_alarm_ids: | ||
continue | ||
speaker = self.hass.data[DATA_SONOS].discovered.get(alarm.zone.uid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alarms.py: speaker = self.hass.data[DATA_SONOS].discovered.get(alarm.zone.uid)
household_coordinator.py: discovered = self.hass.data[DATA_SONOS].discovered
speaker.py: missing_zone = self.hass.data[DATA_SONOS].discovered[uid].zone_name
speaker.py: speaker = self.hass.data[DATA_SONOS].discovered.get(uid)
speaker.py: slave = self.hass.data[DATA_SONOS].discovered.get(slave_uid)
speaker.py: speakers = self.hass.data[DATA_SONOS].discovered.values()
switch.py: self.speaker = self.hass.data[DATA_SONOS].discovered.get(
In a future PR we should hide this implementation detail from the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, absolutely. Need to think about how to best clean this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing:
Added a new alarm in app, verified it showed up
Added a new alarm in app, verified it showed up
Enabled/disabled alarm 1 from hass
Enabled/disabled alarm 2 from hass
Adjusted alarm 1 from hass service
Waited for it to go off
Disabled alarm 1
Reenabled alarm 1
Deleted alarm 1
Deleted alarm 2
verified entities cleared
馃憤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed change
Refactors the alarm and favorites handling in Sonos to utilize various update identifiers to avoid race conditions.
Each Sonos device keeps track of alarms and favorites locally and may not always be perfectly in sync when we query them as updates propagate between the devices. This ensures we're only processing the latest known update and will ignore stale data.
This also bumps
soco
as it's a dependency for this change (SoCo/SoCo#851). Othersoco
changes should be non-breaking: https://github.com/SoCo/SoCo/milestone/21?closed=1. This also fixes #52950 via dependency upgrade.Type of change
Additional information
Checklist
black --fast homeassistant tests
)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
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: