Skip to content

fix(sonos): Improved handling of bonded set membership changes. #2302

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(sonos): Improved handling of bonded set membership changes. #2302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 32 additions & 17 deletions drivers/SmartThings/sonos/src/api/event_handlers.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
local capabilities = require "st.capabilities"
local swGenCapability = capabilities["stus.softwareGeneration"]

local log = require "log"

local st_utils = require "st.utils"

local PlayerFields = require "fields".SonosPlayerFields

local CapEventHandlers = {}

CapEventHandlers.PlaybackStatus = {
Expand All @@ -12,41 +16,52 @@ CapEventHandlers.PlaybackStatus = {
Playing = "PLAYBACK_STATE_PLAYING",
}

local function _do_emit(device, attribute_event)
Copy link
Member Author

@dljsjr dljsjr Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we collapsed everything down to this helper function is because certain activity payloads that come in on a single speaker will contain information about all Players in a group/set/zone/etc., and we will dispatch to all appropriate device records that can be associated with one of the payload entries.

Emitting information about that speaker if it's known to be part of a bonded set would lead to the device record being marked as Online in certain situations, which we don't want.

local bonded = device:get_field(PlayerFields.BONDED)
if not bonded then
device:emit_event(attribute_event)
end
end

function CapEventHandlers.handle_sw_gen(device, sw_gen)
_do_emit(device, swGenCapability.generation(string.format("%s", sw_gen)))
end

function CapEventHandlers.handle_player_volume(device, new_volume, is_muted)
device:emit_event(capabilities.audioVolume.volume(new_volume))
_do_emit(device, capabilities.audioVolume.volume(new_volume))
if is_muted then
device:emit_event(capabilities.audioMute.mute.muted())
_do_emit(device, capabilities.audioMute.mute.muted())
else
device:emit_event(capabilities.audioMute.mute.unmuted())
_do_emit(device, capabilities.audioMute.mute.unmuted())
end
end

function CapEventHandlers.handle_group_volume(device, new_volume, is_muted)
device:emit_event(capabilities.mediaGroup.groupVolume(new_volume))
_do_emit(device, capabilities.mediaGroup.groupVolume(new_volume))
if is_muted then
device:emit_event(capabilities.mediaGroup.groupMute.muted())
_do_emit(device, capabilities.mediaGroup.groupMute.muted())
else
device:emit_event(capabilities.mediaGroup.groupMute.unmuted())
_do_emit(device, capabilities.mediaGroup.groupMute.unmuted())
end
end

function CapEventHandlers.handle_group_role_update(device, group_role)
device:emit_event(capabilities.mediaGroup.groupRole(group_role))
_do_emit(device, capabilities.mediaGroup.groupRole(group_role))
end

function CapEventHandlers.handle_group_coordinator_update(device, coordinator_id)
device:emit_event(capabilities.mediaGroup.groupPrimaryDeviceId(coordinator_id))
_do_emit(device, capabilities.mediaGroup.groupPrimaryDeviceId(coordinator_id))
end

function CapEventHandlers.handle_group_id_update(device, group_id)
device:emit_event(capabilities.mediaGroup.groupId(group_id))
_do_emit(device, capabilities.mediaGroup.groupId(group_id))
end

function CapEventHandlers.handle_group_update(device, group_info)
local groupRole, groupPrimaryDeviceId, groupId = table.unpack(group_info)
device:emit_event(capabilities.mediaGroup.groupRole(groupRole))
device:emit_event(capabilities.mediaGroup.groupPrimaryDeviceId(groupPrimaryDeviceId))
device:emit_event(capabilities.mediaGroup.groupId(groupId))
_do_emit(device, capabilities.mediaGroup.groupRole(groupRole))
_do_emit(device, capabilities.mediaGroup.groupPrimaryDeviceId(groupPrimaryDeviceId))
_do_emit(device, capabilities.mediaGroup.groupId(groupId))
end

function CapEventHandlers.handle_audio_clip_status(device, clips)
Expand All @@ -61,11 +76,11 @@ end

function CapEventHandlers.handle_playback_status(device, playback_state)
if playback_state == CapEventHandlers.PlaybackStatus.Playing then
device:emit_event(capabilities.mediaPlayback.playbackStatus.playing())
_do_emit(device, capabilities.mediaPlayback.playbackStatus.playing())
elseif playback_state == CapEventHandlers.PlaybackStatus.Idle then
device:emit_event(capabilities.mediaPlayback.playbackStatus.stopped())
_do_emit(device, capabilities.mediaPlayback.playbackStatus.stopped())
elseif playback_state == CapEventHandlers.PlaybackStatus.Paused then
device:emit_event(capabilities.mediaPlayback.playbackStatus.paused())
_do_emit(device, capabilities.mediaPlayback.playbackStatus.paused())
elseif playback_state == CapEventHandlers.PlaybackStatus.Buffering then
-- TODO the DTH doesn't currently do anything w/ buffering;
-- might be worth figuring out what to do with this in the future.
Expand All @@ -74,7 +89,7 @@ function CapEventHandlers.handle_playback_status(device, playback_state)
end

function CapEventHandlers.update_favorites(device, new_favorites)
device:emit_event(capabilities.mediaPresets.presets(new_favorites))
_do_emit(device, capabilities.mediaPresets.presets(new_favorites))
end

function CapEventHandlers.handle_playback_metadata_update(device, metadata_status_body)
Expand Down Expand Up @@ -128,7 +143,7 @@ function CapEventHandlers.handle_playback_metadata_update(device, metadata_statu
end

if type(audio_track_data.title) == "string" then
device:emit_event(capabilities.audioTrackData.audioTrackData(audio_track_data))
_do_emit(device, capabilities.audioTrackData.audioTrackData(audio_track_data))
end
end

Expand Down
46 changes: 31 additions & 15 deletions drivers/SmartThings/sonos/src/api/sonos_connection.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,12 @@ local function _open_coordinator_socket(sonos_conn, household_id, self_player_id
return
end

_, err =
Router.open_socket_for_player(household_id, coordinator_id, coordinator.websocketUrl, api_key)
_, err = Router.open_socket_for_player(
household_id,
coordinator_id,
coordinator.player.websocketUrl,
api_key
)
if err ~= nil then
log.error(
string.format(
Expand Down Expand Up @@ -302,10 +306,13 @@ end
--- @return SonosConnection
function SonosConnection.new(driver, device)
log.debug(string.format("Creating new SonosConnection for %s", device.label))
local self = setmetatable(
{ driver = driver, device = device, _listener_uuids = {}, _initialized = false, _reconnecting = false },
SonosConnection
)
local self = setmetatable({
driver = driver,
device = device,
_listener_uuids = {},
_initialized = false,
_reconnecting = false,
}, SonosConnection)

-- capture the label here in case something goes wonky like a callback being fired after a
-- device is removed
Expand Down Expand Up @@ -358,19 +365,28 @@ function SonosConnection.new(driver, device)
local household_id, current_coordinator =
self.driver.sonos:get_coordinator_for_device(self.device)
local _, player_id = self.driver.sonos:get_player_for_device(self.device)
self.driver.sonos:update_household_info(header.householdId, body, self.device)
self.driver.sonos:update_household_info(header.householdId, body, self.driver)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seems odd, but looking at the function definition change, it looks like the device was just an extra arg before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks weird on the surface.

before the OAuth refactor, this method would conditionally take a device record as a flag that it needed to perform some updates. Which was a bad heuristic, so when I refactored it, I got rid of that heuristic, but forgot to remove it from the call sites; Lua being Lua, it didn't throw any errors or cause any problems.

With the refactor for the bonded sets, we changed it so that it always takes a driver record to be able to do some tracking of bonded/unbonded state transitions, hence the updates to the call sites.

self.driver.sonos:update_device_record_from_state(header.householdId, self.device)
local _, updated_coordinator = self.driver.sonos:get_coordinator_for_device(self.device)

local bonded = self.device:get_field(PlayerFields.BONDED)
if bonded then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be combined with the if not bonded below just to simplify the logic?

Copy link
Member Author

@dljsjr dljsjr Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easily; we want to conditionally call :stop(), but then always call cleanup_unused_sockets, and we want to stop the bonded player entirely before calling that function.

The reason being is that cleanup_unused_sockets executes the cleanup logic for when the set of known Coordinators change, so that we get rid of certain namespace subscriptions and connections if a player that was a coordinator is no longer a coordinator.

This cleanup logic is applicable on any Group membership update for the topology regardless of whether or not bonded set membership shifts.

After we do the cleanup, we then want to validate our own coordinator connection (if our coordinator changed) and subscriptions only in the case that we're not part of a bonded set.

self:stop()
end

Router.cleanup_unused_sockets(self.driver)

if not self:coordinator_running() then
--TODO this is not infallible
_open_coordinator_socket(self, household_id, player_id)
end
if not bonded then
if not self:coordinator_running() then
--TODO this is not infallible
_open_coordinator_socket(self, household_id, player_id)
end

if current_coordinator ~= updated_coordinator then
self:refresh_subscriptions()
if current_coordinator ~= updated_coordinator then
self:refresh_subscriptions()
end
else
self.device:offline()
end
elseif header.type == "playerVolume" then
log.trace(string.format("PlayerVolume type message for %s", device_name))
Expand Down Expand Up @@ -477,7 +493,7 @@ function SonosConnection.new(driver, device)
return
end

local url_ip = lb_utils.force_url_table(coordinator_player.websocketUrl).host
local url_ip = lb_utils.force_url_table(coordinator_player.player.websocketUrl).host
local base_url = lb_utils.force_url_table(
string.format("https://%s:%s", url_ip, SonosApi.DEFAULT_SONOS_PORT)
)
Expand Down Expand Up @@ -590,7 +606,7 @@ function SonosConnection:coordinator_running()
)
)
end
return type(unique_key) == "string" and Router.is_connected(unique_key) and self._initialized
return type(unique_key) == "string" and Router.is_connected(unique_key)
end

function SonosConnection:refresh_subscriptions(maybe_reply_tx)
Expand Down
2 changes: 1 addition & 1 deletion drivers/SmartThings/sonos/src/api/sonos_ssdp_discovery.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ function sonos_ssdp.spawn_persistent_ssdp_task()
if info_to_send then
if not (info_to_send.discovery_info and info_to_send.discovery_info.device) then
log.error_with(
{ hub_logs = true },
{ hub_logs = false },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we don't want to include this in hub logs?

Copy link
Member Author

@dljsjr dljsjr Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be chatty, and we've been asked by trim down the amount of logging that the LAN Edge Drivers forward to hub-core. It was set to true during initial development of the refactor for debugging purposes but doesn't really need to be that way anymore. With the ability to set the broker.promoted_driver_logs flag in LD to promote all logs for a driver, there isn't as much need for this in general unless it's on a path that is really pathological, IMO.

st_utils.stringify_table(info_to_send, "Sonos Discovery Info has unexpected structure")
)
return
Expand Down
1 change: 1 addition & 0 deletions drivers/SmartThings/sonos/src/fields.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ local Fields = {}
Fields.SonosPlayerFields = {
_IS_INIT = "init",
_IS_SCANNING = "scanning",
BONDED = "bonded",
CONNECTION = "conn",
UNIQUE_KEY = "unique_key",
HOUSEHOLD_ID = "householdId",
Expand Down
4 changes: 2 additions & 2 deletions drivers/SmartThings/sonos/src/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@ if api_version < 14 then
driver:start_ssdp_event_task()
end

log.info "Starting Sonos run loop"
log.info("Starting Sonos run loop")
driver:run()
log.info "Exiting Sonos run loop"
log.info("Exiting Sonos run loop")
10 changes: 6 additions & 4 deletions drivers/SmartThings/sonos/src/lifecycle_handlers.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,12 @@ function SonosDriverLifecycleHandlers.initialize_device(driver, device)
return
end
log.error_with(
{ hub_logs = true },
"Error handling Sonos player initialization: %s, error code: %s",
error,
(error_code or "N/A")
{ hub_logs = false },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about not including this in hub logs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be chatty as well, and based on logs that I've looked at from the fleet, it's almost always only hit by non-primary bonded players, so it's not providing useful information.

string.format(
"Error handling Sonos player initialization: %s, error code: %s",
error,
(error_code or "N/A")
)
)
end
end
Expand Down
61 changes: 47 additions & 14 deletions drivers/SmartThings/sonos/src/sonos_driver.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,27 @@ local ONE_HOUR_IN_SECONDS = 3600
---@field private waiting_for_oauth_token boolean
---@field private startup_state_received boolean
---@field private devices_waiting_for_startup_state SonosDevice[]
---@field package bonded_devices table<string, boolean> map of Device device_network_id to a boolean indicating if the device is currently known as a bonded device.
---
---@field public ssdp_task SonosPersistentSsdpTask?
---@field private ssdp_event_thread_handle table?
local SonosDriver = {}

---@param device SonosDevice
function SonosDriver:update_bonded_device_tracking(device)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in this function is very confusing to me, can you explain what is intended to be tracked here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent is to track when a device record's bonded status changes.

The device's current bonded status is tracked on the device itself via the BONDED field, which we always update as soon as possible so that we can do stuff like short circuiting emits so that we don't mark an offline device as online.

The driver shadows this information in a map that is updated independently and with less urgency, so that it can be used to look up the previous state before updating itself.

This lets us determine if a device's bonded status changes. If a device becomes bonded, we mark it offline. If a device was bonded and is no longer bonded, we re-initialize it.

local already_bonded = self.bonded_devices[device.device_network_id]
local currently_bonded = device:get_field(PlayerFields.BONDED)
self.bonded_devices[device.device_network_id] = currently_bonded

if currently_bonded and not already_bonded then
device:offline()
end

if already_bonded and not currently_bonded then
SonosDriverLifecycleHandlers.initialize_device(self, device)
end
end

function SonosDriver:has_received_startup_state()
return self.startup_state_received
end
Expand Down Expand Up @@ -127,13 +143,13 @@ end
function SonosDriver:handle_augmented_store_delete(update_key)
if update_key == "endpointAppInfo" then
if update_key == "endpointAppInfo" then
log.trace "deleting endpoint app info"
log.trace("deleting endpoint app info")
self.oauth.endpoint_app_info = nil
elseif update_key == "sonosOAuthToken" then
log.trace "deleting OAuth Token"
log.trace("deleting OAuth Token")
self.oauth.token = nil
elseif update_key == "force_oauth" then
log.trace "deleting Force OAuth"
log.trace("deleting Force OAuth")
self.oauth.force_oauth = nil
else
log.debug(string.format("received delete of unexpected key: %s", update_key))
Expand Down Expand Up @@ -423,9 +439,15 @@ local function make_ssdp_event_handler(
local event, recv_err = discovery_event_subscription:receive()

if event then
local mac_addr = utils.extract_mac_addr(event.discovery_info.device)
local unique_key = utils.sonos_unique_key_from_ssdp(event.ssdp_info)
if
event.force_refresh or not (unauthorized[unique_key] or discovered[unique_key])
event.force_refresh
or not (
unauthorized[unique_key]
or discovered[unique_key]
or driver.bonded_devices[mac_addr]
)
then
local _, api_key = driver:check_auth(event)
local success, handle_err, err_code =
Expand All @@ -435,7 +457,7 @@ local function make_ssdp_event_handler(
unauthorized[unique_key] = event
end
log.warn_with(
{ hub_logs = true },
{ hub_logs = false },
string.format("Failed to handle discovered speaker: %s", handle_err)
)
else
Expand Down Expand Up @@ -483,12 +505,22 @@ function SonosDriver:handle_player_discovery_info(api_key, info, device)
-- speaker in a bonded set (e.g. a home theater system, a stereo pair, etc).
-- These aren't the same as speaker groups, and bonded speakers can't be controlled
-- via websocket at all. So we ignore all bonded non-primary speakers
if #info.ssdp_info.group_id == 0 then
return nil,
string.format(
"Player %s is a non-primary bonded Sonos device, ignoring",
info.discovery_info.device.name
)
-- if then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented code

-- return nil,
-- string.format(
-- "Player %s is a non-primary bonded Sonos device, ignoring",
-- info.discovery_info.device.name
-- )
-- end

local discovery_info_mac_addr = utils.extract_mac_addr(info.discovery_info.device)
local bonded = (#info.ssdp_info.group_id == 0)
self.bonded_devices[discovery_info_mac_addr] = bonded

local maybe_device = self:get_device_by_dni(discovery_info_mac_addr)
if maybe_device then
maybe_device:set_field(PlayerFields.BONDED, bonded, { persist = false })
self:update_bonded_device_tracking(maybe_device)
end

api_key = api_key or self:get_fallback_api_key()
Expand Down Expand Up @@ -543,7 +575,7 @@ function SonosDriver:handle_player_discovery_info(api_key, info, device)
end

--- @cast response SonosGroupsResponseBody
self.sonos:update_household_info(info.ssdp_info.household_id, response)
self.sonos:update_household_info(info.ssdp_info.household_id, response, self)

local device_to_update, device_mac_addr

Expand All @@ -565,7 +597,7 @@ function SonosDriver:handle_player_discovery_info(api_key, info, device)
if not (info and info.discovery_info and info.discovery_info.device) then
return nil, st_utils.stringify_table(info, "Sonos Discovery Info has unexpected structure")
end
device_mac_addr = utils.extract_mac_addr(info.discovery_info.device)
device_mac_addr = discovery_info_mac_addr
end

if not device_to_update then
Expand All @@ -578,7 +610,7 @@ function SonosDriver:handle_player_discovery_info(api_key, info, device)
if device_to_update then
self.dni_to_device_id[device_mac_addr] = device_to_update.id
self.sonos:associate_device_record(device_to_update, info)
else
elseif not bonded then
local name = info.discovery_info.device.name
or info.discovery_info.device.modelDisplayName
or "Unknown Sonos Player"
Expand Down Expand Up @@ -631,6 +663,7 @@ function SonosDriver.new_driver_template()
waiting_for_oauth_token = false,
startup_state_received = false,
devices_waiting_for_startup_state = {},
bonded_devices = utils.new_mac_address_keyed_table(),
dni_to_device_id = utils.new_mac_address_keyed_table(),
lifecycle_handlers = SonosDriverLifecycleHandlers,
capability_handlers = {
Expand Down
Loading
Loading