Matter Energy: Fix non- JSON serializable fields #2418
Conversation
Keys in datastore tables need to be strings in order to be JSON serializable. SUPPORTED_EVSE_MODES_MAP and SUPPORTED_DEVICE_ENERGY_MANAGEMENT_MODES_MAP need to be updated.
|
Invitation URL: |
Test Results 70 files 451 suites 0s ⏱️ Results for commit abc9d86. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against abc9d86 |
| table.insert(supportedEvseModes, mode.elements.label.value) | ||
| end | ||
| supportedEvseModesMap[ib.endpoint_id] = supportedEvseModes | ||
| supportedEvseModesMap[tostring(ib.endpoint_id)] = supportedEvseModes |
There was a problem hiding this comment.
I think these changes make sense and address the immediate issue, but there is something that I think we could improve here: would we be able to use the set_field_for_endpoint and get_field_for_endpoint commands here instead of directly saving all this endpoint information to a table?
The set/get_field_for_endpoint command are already used throughout our other drivers, and in those cases we don't run into the issue of the index being a string because the "stringification" of the key value is handled by the functions themselves. Here are the functions copied from the matter-switch driver (although they are used in many drivers):
local function get_field_for_endpoint(device, field, endpoint)
return device:get_field(string.format("%s_%d", field, endpoint))
end
local function set_field_for_endpoint(device, field, endpoint, value, additional_params)
device:set_field(string.format("%s_%d", field, endpoint), value, additional_params)
end
There was a problem hiding this comment.
That would definitely be nice, I just wonder if that would cause issues given that it would be a different format than what is currently used on devices in the field. I supposed that it could be avoided by starting with an empty list in the supported modes handlers rather than adding to the existing lists.
There was a problem hiding this comment.
Come to think of it, maybe that's how it should be done, since, although probably unlikely, a device could potentially not support a particular mode after supporting it previously, perhaps following a FW update. So it might make sense to start fresh each time the SupportedModes attributes are received.
There was a problem hiding this comment.
Never mind, the driver already accounts for that. But still, I like the idea of using get/set_field_for_endpoint and will add an additional commit to this PR with an implementation
There was a problem hiding this comment.
Updated in abc9d86
| local ep = component_to_endpoint(device, cmd.component) | ||
| local supportedDeviceEnergyMgmtModesMap = device:get_field(SUPPORTED_DEVICE_ENERGY_MANAGEMENT_MODES_MAP) | ||
| local supportedDeviceEnergyMgmtModes = supportedDeviceEnergyMgmtModesMap[ep] or {} | ||
| local supportedDeviceEnergyMgmtModes = get_field_for_endpoint(device, SUPPORTED_DEVICE_ENERGY_MANAGEMENT_MODES, ep) |
There was a problem hiding this comment.
| local supportedDeviceEnergyMgmtModes = get_field_for_endpoint(device, SUPPORTED_DEVICE_ENERGY_MANAGEMENT_MODES, ep) | |
| local supportedDeviceEnergyMgmtModes = get_field_for_endpoint(device, SUPPORTED_DEVICE_ENERGY_MANAGEMENT_MODES, ep) or {} |
| local ep = component_to_endpoint(device, cmd.component) | ||
| local supportedEvseModesMap = device:get_field(SUPPORTED_EVSE_MODES_MAP) | ||
| local supportedEvseModes = supportedEvseModesMap[ep] or {} | ||
| local supportedEvseModes = get_field_for_endpoint(device, SUPPORTED_EVSE_MODES, ep) |
There was a problem hiding this comment.
| local supportedEvseModes = get_field_for_endpoint(device, SUPPORTED_EVSE_MODES, ep) | |
| local supportedEvseModes = get_field_for_endpoint(device, SUPPORTED_EVSE_MODES, ep) or {} |
Keys in datastore tables need to be strings in order to be JSON serializable.
SUPPORTED_EVSE_MODES_MAPandSUPPORTED_DEVICE_ENERGY_MANAGEMENT_MODES_MAPneed to be updated.