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.

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

Fix SmartThings Cover Set Position (for window shades) #96612

Merged
merged 2 commits into from Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 22 additions & 6 deletions homeassistant/components/smartthings/cover.py
Expand Up @@ -62,7 +62,11 @@ def get_capabilities(capabilities: Sequence[str]) -> Sequence[str] | None:
# Must have one of the min_required
if any(capability in capabilities for capability in min_required):
# Return all capabilities supported/consumed
return min_required + [Capability.battery, Capability.switch_level]
return min_required + [
Capability.battery,
Capability.switch_level,
Capability.window_shade_level,
]

return None

Expand All @@ -74,12 +78,16 @@ def __init__(self, device):
"""Initialize the cover class."""
super().__init__(device)
self._device_class = None
self._current_cover_position = None
self._state = None
self._state_attrs = None
self._attr_supported_features = (
CoverEntityFeature.OPEN | CoverEntityFeature.CLOSE
)
if Capability.switch_level in device.capabilities:
if (
Capability.switch_level in device.capabilities
or Capability.window_shade_level in device.capabilities
):
self._attr_supported_features |= CoverEntityFeature.SET_POSITION

async def async_close_cover(self, **kwargs: Any) -> None:
Expand All @@ -103,7 +111,12 @@ async def async_set_cover_position(self, **kwargs: Any) -> None:
if not self.supported_features & CoverEntityFeature.SET_POSITION:
return
# Do not set_status=True as device will report progress.
await self._device.set_level(kwargs[ATTR_POSITION], 0)
if Capability.window_shade_level in self._device.capabilities:
await self._device.set_window_shade_level(
kwargs[ATTR_POSITION], set_status=False
)
else:
await self._device.set_level(kwargs[ATTR_POSITION], set_status=False)

async def async_update(self) -> None:
"""Update the attrs of the cover."""
Expand All @@ -117,6 +130,11 @@ async def async_update(self) -> None:
self._device_class = CoverDeviceClass.GARAGE
self._state = VALUE_TO_STATE.get(self._device.status.door)

if Capability.window_shade_level in self._device.capabilities:
self._current_cover_position = self._device.status.shade_level
elif Capability.switch_level in self._device.capabilities:
self._current_cover_position = self._device.status.level

self._state_attrs = {}
battery = self._device.status.attributes[Attribute.battery].value
if battery is not None:
Expand All @@ -142,9 +160,7 @@ def is_closed(self) -> bool | None:
@property
def current_cover_position(self) -> int | None:
"""Return current position of cover."""
if not self.supported_features & CoverEntityFeature.SET_POSITION:
return None
return self._device.status.level
return self._current_cover_position

@property
def device_class(self) -> CoverDeviceClass | None:
Expand Down
2 changes: 1 addition & 1 deletion homeassistant/components/smartthings/manifest.json
Expand Up @@ -30,5 +30,5 @@
"documentation": "https://www.home-assistant.io/integrations/smartthings",
"iot_class": "cloud_push",
"loggers": ["httpsig", "pysmartapp", "pysmartthings"],
"requirements": ["pysmartapp==0.3.3", "pysmartthings==0.7.6"]
"requirements": ["pysmartapp==0.3.5", "pysmartthings==0.7.8"]
}
4 changes: 2 additions & 2 deletions requirements_all.txt
Expand Up @@ -2006,10 +2006,10 @@ pysma==0.7.3
pysmappee==0.2.29

# homeassistant.components.smartthings
pysmartapp==0.3.3
pysmartapp==0.3.5

# homeassistant.components.smartthings
pysmartthings==0.7.6
pysmartthings==0.7.8

# homeassistant.components.edl21
pysml==0.0.12
Expand Down
4 changes: 2 additions & 2 deletions requirements_test_all.txt
Expand Up @@ -1492,10 +1492,10 @@ pysma==0.7.3
pysmappee==0.2.29

# homeassistant.components.smartthings
pysmartapp==0.3.3
pysmartapp==0.3.5

# homeassistant.components.smartthings
pysmartthings==0.7.6
pysmartthings==0.7.8

# homeassistant.components.edl21
pysml==0.0.12
Expand Down
37 changes: 35 additions & 2 deletions tests/components/smartthings/test_cover.py
Expand Up @@ -113,8 +113,10 @@ async def test_close(hass: HomeAssistant, device_factory) -> None:
assert state.state == STATE_CLOSING


async def test_set_cover_position(hass: HomeAssistant, device_factory) -> None:
"""Test the cover sets to the specific position."""
async def test_set_cover_position_switch_level(
hass: HomeAssistant, device_factory
) -> None:
"""Test the cover sets to the specific position for legacy devices that use Capability.switch_level."""
# Arrange
device = device_factory(
"Shade",
Expand All @@ -140,6 +142,37 @@ async def test_set_cover_position(hass: HomeAssistant, device_factory) -> None:
assert device._api.post_device_command.call_count == 1 # type: ignore


async def test_set_cover_position(hass: HomeAssistant, device_factory) -> None:
"""Test the cover sets to the specific position."""
# Arrange
device = device_factory(
"Shade",
[Capability.window_shade, Capability.battery, Capability.window_shade_level],
{
Attribute.window_shade: "opening",
Attribute.battery: 95,
Attribute.shade_level: 10,
},
)
await setup_platform(hass, COVER_DOMAIN, devices=[device])
# Act
await hass.services.async_call(
COVER_DOMAIN,
SERVICE_SET_COVER_POSITION,
{ATTR_POSITION: 50, "entity_id": "all"},
blocking=True,
)

state = hass.states.get("cover.shade")
# Result of call does not update state
assert state.state == STATE_OPENING
assert state.attributes[ATTR_BATTERY_LEVEL] == 95
assert state.attributes[ATTR_CURRENT_POSITION] == 10
# Ensure API called

assert device._api.post_device_command.call_count == 1 # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this test is following the same pattern as the existing tests, but wouldn't it make sense to check the parameters of post_device_command also to check the correct command and correct parameters are used?

This can be done in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I don't really like the leaky abstraction of digging into the guts of the library to mock, so will explore mocking the API actually used by HA in the future.



async def test_set_cover_position_unsupported(
hass: HomeAssistant, device_factory
) -> None:
Expand Down