Skip to content
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

Matter Switch: fix endpoint to component mapping #986

Merged
merged 6 commits into from
Sep 26, 2023
Merged

Matter Switch: fix endpoint to component mapping #986

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b637d9c
Matter Switch: fix endpoint to component mapping
ctowns Sep 25, 2023
a9e0c3a
fixup! move mapping to 'init', remove added
ctowns Sep 25, 2023
d70a309
fixup! move mapping and profile switch to init, fix unit tests
ctowns Sep 25, 2023
5ae1591
fixup! fix unit test name
ctowns Sep 25, 2023
2ec1713
fixup! add more error logs, simplify loop
ctowns Sep 26, 2023
f8aa183
fixup! add cluster to find_default_endpoint
ctowns Sep 26, 2023
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
64 changes: 43 additions & 21 deletions drivers/SmartThings/matter-switch/src/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ local COLOR_TEMPERATURE_KELVIN_MIN = 1
local COLOR_TEMPERATURE_MIRED_MAX = CONVERSION_CONSTANT/COLOR_TEMPERATURE_KELVIN_MIN
local COLOR_TEMPERATURE_MIRED_MIN = CONVERSION_CONSTANT/COLOR_TEMPERATURE_KELVIN_MAX

local ENDPOINT_TO_COMPONENT_MAP = "__endpoint_to_component_map"
ctowns marked this conversation as resolved.
Show resolved Hide resolved

local function convert_huesat_st_to_matter(val)
return math.floor((val * 0xFE) / 100.0 + 0.5)
end
Expand All @@ -49,23 +51,47 @@ local function find_default_endpoint(device, component)
return res
end

local function component_to_endpoint(device, component_id)
-- Assumes matter endpoint layout is sequentional starting at 1.
local ep_num = component_id:match("switch(%d)")
return ep_num and tonumber(ep_num) or find_default_endpoint(device, component_id)
local function create_endpoint_to_component_map(device)
ctowns marked this conversation as resolved.
Show resolved Hide resolved
local switch_eps = device:get_endpoints(clusters.OnOff.ID)
table.sort(switch_eps)

local endpoint_map = {}
local current_component_number = 1

-- For switch devices, the profile components follow the naming convention "switch%d",
-- with the exception of "main" being the first component. Each component will then map
-- to the next lowest endpoint that hasn't been mapped yet.
for _, ep in ipairs(switch_eps) do
if current_component_number == 1 then
endpoint_map[ep] = "main"
else
endpoint_map[ep] = string.format("switch%d", current_component_number)
end
current_component_number = current_component_number + 1
ctowns marked this conversation as resolved.
Show resolved Hide resolved
end

device:set_field(ENDPOINT_TO_COMPONENT_MAP, endpoint_map, {persist = true})
end

local function component_to_endpoint(device, component_name)
local map = device:get_field(ENDPOINT_TO_COMPONENT_MAP) or {}
for ep, component in pairs(map) do
if component == component_name then return ep end
end
return find_default_endpoint(device, component_name)
end
ctowns marked this conversation as resolved.
Show resolved Hide resolved

local function endpoint_to_component(device, ep)
local switch_comp = string.format("switch%d", ep)
if device.profile.components[switch_comp] ~= nil then
return switch_comp
else
return "main"
local map = device:get_field(ENDPOINT_TO_COMPONENT_MAP) or {}
if map[ep] and device.profile.components[map[ep]] then
return map[ep]
end
return "main"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think what we probably want to do here is, if we have any endpoint to component mapping done at all, to ignore messages from a non-mapped endpoint, rather than emit them from main

I'm thinking of a situation where you have more switches than we have profiles for. Anything from endpoint > X will change the state of "main" which is also a real component that we have mapped to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The emit_event_for_endpoint function expects the endpoint_to_component function to return something, or else we would run into an issue in emit_component_event when we try to use the nil value for the component. So, I think there needs to be some kind of default value, even if it is not ideal.

I'm thinking of a situation where you have more switches than we have profiles for. Anything from endpoint > X will change the state of "main" which is also a real component that we have mapped to.

I think this is a valid problem that would be caused by this. The solution would be to add another profile to account for the number of switches. Ideally we'd be able to quickly see the error in the logs since it is logged if a device has more than our supported number of switches

Copy link
Contributor

Choose a reason for hiding this comment

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

if you feel like digging into it, what I did with buttons was use parent-child in the case that I didn't have a profile configured for that number of components

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended to handle the base case or the unexpected case? I would have expected that all expected cases, including the base case with just one endpoint, would be handled by the map.

If this is handling the unexpected case, we need to be louder here, and should avoid affecting the operation of other components. Regardless of the answer to the above, I think that this is how we need to be handling the unexpected case.

Copy link
Contributor Author

@ctowns ctowns Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if you feel like digging into it, what I did with buttons was use parent-child in the case that I didn't have a profile configured for that number of components

@greens What are the tradeoffs between using parent-child vs. making a new profile? My understanding on how we would handle this is that we support up to 7 switches, but if a device comes along that has more we'd add another profile for number of switches and it would work. We'd know when a device needs a new profile because of the error() in initialize_switch()when there are more than the max number of switches. I think adding parent-child could be done in a separate PR and I think it could be a good idea given the limitation of our static profiles, but would that mean we'd rely on parent-child instead of creating a new static profile for devices with >7 switches?

I am also not sure what the market looks like, but a cursory glance seems like bumping our support up to 8 might cover most use cases.

Is this intended to handle the base case or the unexpected case? I would have expected that all expected cases, including the base case with just one endpoint, would be handled by the map.

If this is handling the unexpected case, we need to be louder here, and should avoid affecting the operation of other components. Regardless of the answer to the above, I think that this is how we need to be handling the unexpected case.

@gharveymn The endpoint map will be created for all devices, regardless of how many endpoints they have (including base case with just one endpoint). For all expected cases (up to 7 switches), each endpoint will be mapped to its own component. Mapping excess endpoints to main is not ideal, but they must be mapped to something (see my comment above). I could add a log here for cases where an excess endpoint is mapped to main so it is clear that we are hitting this unexpected case.

We could potentially make some changes to the lua libs to allow us to ignore unsupported "excess endpoints", but they would be another PR outside of the drivers. Depending on how we decide to handle the unexpected cases (i.e. with only static profiles, or with the parent-child relationship Steven mentioned above), I think the lua lib changes to allow for ignored endpoints could be worthwhile.

end

local function device_init(driver, device)
log.info_with({hub_logs=true}, "device init")
create_endpoint_to_component_map(device)
device:set_component_to_endpoint_fn(component_to_endpoint)
device:set_endpoint_to_component_fn(endpoint_to_component)
device:subscribe()
Expand All @@ -82,18 +108,14 @@ local function do_configure(driver, device)
-- where devices with multiple endpoints with the same device type cannot be detected
local switch_eps = device:get_endpoints(clusters.OnOff.ID)
local num_switch_eps = #switch_eps
table.sort(switch_eps)
--Default MCD switch handling depends on consecutive endpoint numbering
if num_switch_eps == switch_eps[num_switch_eps] then
if num_switch_eps > 1 then
device:try_update_metadata({profile = string.format("switch-%d", math.min(num_switch_eps, MAX_MULTI_SWITCH_EPS))})
end
if num_switch_eps > MAX_MULTI_SWITCH_EPS then
error(string.format(
"Matter multi switch device will not function. Profile doesn't exist with %d components",
num_switch_eps
))
end
if num_switch_eps > 1 then
device:try_update_metadata({profile = string.format("switch-%d", math.min(num_switch_eps, MAX_MULTI_SWITCH_EPS))})
Copy link
Contributor

Choose a reason for hiding this comment

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

As it was used as such in the past, it could have been useful to define switch-%d as a constant, to be shared across all drivers, to ensure the format would be consistent across.

end
if num_switch_eps > MAX_MULTI_SWITCH_EPS then
error(string.format(
"Matter multi switch device will have limited function. Profile doesn't exist with %d components",
num_switch_eps
ctowns marked this conversation as resolved.
Show resolved Hide resolved
))
end
end

Expand Down
1 change: 0 additions & 1 deletion drivers/SmartThings/matter-switch/src/test/test_matter_switch.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ local function test_init()
end
test.socket.matter:__expect_send({mock_device_no_hue_sat.id, subscribe_request})
test.mock_device.add_test_device(mock_device_no_hue_sat)

end
test.set_test_init_function(test_init)

Expand Down
78 changes: 78 additions & 0 deletions drivers/SmartThings/matter-switch/src/test/test_multi_switch.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,52 @@ local mock_2switch = test.mock_device.build_test_matter_device({
}
})

local mock_3switch_non_sequential = test.mock_device.build_test_matter_device({
profile = t_utils.get_profile_definition("light-binary.yml"),
manufacturer_info = {
vendor_id = 0x0000,
product_id = 0x0000,
},
endpoints = {
{
endpoint_id = 0,
clusters = {
{cluster_id = clusters.Basic.ID, cluster_type = "SERVER"},
},
device_types = {
device_type_id = 0x0016, device_type_revision = 1, -- RootNode
}
},
{
endpoint_id = 10,
clusters = {
{cluster_id = clusters.OnOff.ID, cluster_type = "SERVER"},
},
device_types = {
device_type_id = 0x0100, device_type_revision = 2, -- On/Off Light
}
},
{
endpoint_id = 14,
clusters = {
{cluster_id = clusters.OnOff.ID, cluster_type = "SERVER"},
},
device_types = {
device_type_id = 0x0100, device_type_revision = 2, -- On/Off Light
}
},
{
endpoint_id = 15,
clusters = {
{cluster_id = clusters.OnOff.ID, cluster_type = "SERVER"},
},
device_types = {
device_type_id = 0x0100, device_type_revision = 2, -- On/Off Light
}
},
}
})


local function test_init()
local cluster_subscribe_list = {
Expand All @@ -112,6 +158,9 @@ local function test_init()
local subscribe_request = cluster_subscribe_list[1]:subscribe(mock_2switch)
test.socket.matter:__expect_send({mock_2switch.id, subscribe_request})
test.mock_device.add_test_device(mock_2switch)
local subscribe_request = cluster_subscribe_list[1]:subscribe(mock_3switch_non_sequential)
test.socket.matter:__expect_send({mock_3switch_non_sequential.id, subscribe_request})
test.mock_device.add_test_device(mock_3switch_non_sequential)
end
test.set_test_init_function(test_init)

Expand All @@ -130,6 +179,13 @@ test.register_coroutine_test(
mock_2switch:expect_metadata_update({ provisioning_state = "PROVISIONED" })
end)

test.register_coroutine_test(
"Profile change for 3 switch device with non sequential endpoints", function()
test.socket.device_lifecycle:__queue_receive({ mock_3switch_non_sequential.id, "doConfigure" })
mock_3switch_non_sequential:expect_metadata_update({ profile = "switch-3" })
mock_3switch_non_sequential:expect_metadata_update({ provisioning_state = "PROVISIONED" })
end)

test.register_message_test(
"On command to component switch should send the appropriate commands",
{
Expand All @@ -152,4 +208,26 @@ test.register_message_test(
}
)

test.register_message_test(
"On command to component switch should send the appropriate commands for devices with non-sequential endpoints",
{
{
channel = "capability",
direction = "receive",
message = {
mock_3switch_non_sequential.id,
{ capability = "switch", component = "switch3", command = "on", args = { } }
}
},
{
channel = "matter",
direction = "send",
message = {
mock_3switch_non_sequential.id,
clusters.OnOff.server.commands.On(mock_3switch_non_sequential, 15) -- switch 3 is on endpoint 15
ctowns marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
)

test.run_registered_tests()