Keep the Setup Endpoints answer per connection

pyhap/accessory_driver.py

@@ -1343,12 +1343,13 @@ def _run_characteristic_writes(
                 and HAP_PERMISSION_WRITE_RESPONSE in char.properties[PROP_PERMISSIONS]
             )
             # A new write replaces a former response of this connection. The
-            # request of a readable write response characteristic is not
-            # shared, whatever the setter does, also not with ev subscribers.
+            # request of a readable write response or read back characteristic
+            # is not shared, whatever the setter does, also not with ev
+            # subscribers.
             if value is not None:
                 self._write_responses.get(client_addr, {}).pop(char, None)
             private_write = (
-                write_response
+                (write_response or char.read_back_response)
                 and value is not None
                 and HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS]
             )
@@ -1366,8 +1367,22 @@ def _run_characteristic_writes(
             # the writer reads the answer back, see _store_write_response.
             # client_update_value stored the request as the value, so put
             # back the former one unless the setter set a value of its own.
-            if private_write and char.value == _stored_value(char, value):
-                char.value = previous_value
+            # Whatever the setter of a read back characteristic leaves in the
+            # value is an answer for the writer only, e.g. the Setup Endpoints
+            # answer of a subclass that calls super() and returns nothing.
+            # Neither the former value of a refused write nor the request the
+            # setter left alone is an answer.
+            if private_write:
+                own_value = char.value
+                if char.read_back_response:
+                    if set_result_value is None and own_value not in (
+                        previous_value,
+                        _stored_value(char, value),
+                    ):
+                        set_result_value = own_value
+                    char.value = previous_value
+                elif own_value == _stored_value(char, value):
+                    char.value = previous_value
 
             result: dict[str, Any] = {HAP_REPR_STATUS: set_result}
             if inspect.iscoroutine(set_result_value):
@@ -1381,9 +1396,9 @@ def _run_characteristic_writes(
                         not private_write,
                     )
                 )
-            elif set_result_value is not None and write_response:
-                result[HAP_REPR_VALUE] = self._store_write_response(
-                    char, set_result_value, client_addr
+            elif set_result_value is not None:
+                self._keep_response(
+                    result, char, set_result_value, write_response, client_addr
                 )
 
             results[aid][iid] = result
@@ -1464,11 +1479,11 @@ async def _async_finish_set_characteristics(
             write.char.client_update_finished(
                 write.previous_value, client_addr, notify=write.notify
             )
-            if outcome is not None and write.write_response:
+            if outcome is not None:
                 # A response queued when the connection closes is dropped by
                 # the deferred cleanup in connection_lost.
-                write.result[HAP_REPR_VALUE] = self._store_write_response(
-                    write.char, outcome, client_addr
+                self._keep_response(
+                    write.result, write.char, outcome, write.write_response, client_addr
                 )
         return _set_characteristics_response(results)
 
@@ -1491,6 +1506,25 @@ def _finish_done_write(
             write.previous_value, client_addr, notify=write.notify
         )
 
+    def _keep_response(
+        self,
+        result: dict[str, Any],
+        char: Characteristic,
+        response: HAPValue,
+        write_response: bool,
+        client_addr: tuple[str, int] | None,
+    ) -> None:
+        """Keep what a setter returned as a write response or read back value."""
+        if write_response:
+            result[HAP_REPR_VALUE] = self._store_write_response(
+                char, response, client_addr
+            )
+        elif (
+            char.read_back_response
+            and HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS]
+        ):
+            self._store_write_response(char, response, client_addr)
+
     def _take_prepared_write(self, pid: Any, client_addr: tuple[str, int]) -> bool:
         """Consume the prepared write ``pid`` and return whether it is still valid."""
         if not _is_hap_int(pid):
@@ -1513,8 +1547,9 @@ def _store_write_response(
         response back, until it closes, and it wins over the getter and a later
         ``set_value``. Other connections read the characteristic's own value,
         which the write leaves as it was. A write without ``r`` stays readable
-        for everyone, and a characteristic that also has ``ev`` notifies the
-        written value as any write does.
+        for everyone and notifies ``ev`` subscribers as any write does, unless
+        the characteristic has ``read_back_response``, which keeps the response
+        the same way without a write response.
         """
         readable = HAP_PERMISSION_READ in char.properties[PROP_PERMISSIONS]
         try:

pyhap/camera.py

@@ -7,8 +7,8 @@
 [0. Read the current streaming status]
 1. Sets the SetupEndpoints characteristic to notify the camera about its IP address,
 selected security parameters, etc.
-2. The camera responds to the above by setting the SetupEndpoints with its IP address,
-etc.
+2. The camera answers the write with its IP address, etc., and the client reads the
+answer back from SetupEndpoints.
 3. The client sets the SelectedRTPStreamConfiguration characteristic to notify the
 camera of its prefered audio and video configuration and to initiate the start of the
 streaming.
@@ -349,7 +349,7 @@ def _decode_setup_endpoints(
     )
 
 
-def _setup_endpoints_status(session_id: UUID, status: bytes) -> Any:
+def _setup_endpoints_status(session_id: UUID, status: bytes) -> str:
     """Encode a Setup Endpoints answer that has only a status."""
     return tlv.encode(
         SETUP_TYPES["SESSION_ID"],
@@ -745,12 +745,14 @@ def _create_stream_management(
             "SelectedRTPStreamConfiguration",
             setter_callback=self.set_selected_stream_configuration,
         )
-        management.configure_char(
+        setup_endpoints = management.configure_char(
             "SetupEndpoints",
             setter_callback=lambda value: self.set_endpoints(
                 value, stream_idx=stream_idx
             ),
         )
+        # The answer has the session's SRTP keys, only its controller reads it.
+        setup_endpoints.read_back_response = True
         return management
 
     def _default_recording_configs(
@@ -1748,15 +1750,22 @@ def set_streaming_available(self, stream_idx: int) -> None:
         self._streaming_status[stream_idx] = STREAMING_STATUS["AVAILABLE"]
         self._management[stream_idx].get_characteristic("StreamingStatus").notify()
 
-    def set_endpoints(self, value: str, stream_idx: int | None = None) -> None:
+    def set_endpoints(self, value: str, stream_idx: int | None = None) -> str:
         """Configure streaming endpoints.
 
-        Called when iOS sets the SetupEndpoints ``Characteristic``. The endpoint
-        information for the camera should be set as the current value of SetupEndpoints.
-        A refused write sets the error answer.
+        Called when iOS sets the SetupEndpoints ``Characteristic``. Sets the
+        endpoint information of the camera, or a refusal, as the value of
+        SetupEndpoints and returns it. A subclass that calls ``super()`` and
+        returns nothing still answers. For a controller's write the driver
+        keeps the answer for the connection that wrote and puts the former
+        value back. An async setter must return the answer and must not set
+        the value itself, a value it sets is shared with every controller. A
+        direct ``client_update_value`` leaves the answer as the value.
 
         :param value: The base64-encoded stream session details in TLV format.
         :param value: ``str``
+        :return: The base64-encoded answer in TLV format.
+        :rtype: ``str``
         """
         if stream_idx is None:
             stream_idx = 0
@@ -1774,23 +1783,16 @@ def set_endpoints(self, value: str, stream_idx: int | None = None) -> None:
             ) = _decode_setup_endpoints(session_id, objs)
         except (IndexError, KeyError, ValueError, struct.error) as ex:
             logger.warning("[%s] Refusing the endpoints: %s", session_id, ex)
-            self._management[stream_idx].get_characteristic("SetupEndpoints").set_value(
-                _setup_endpoints_status(session_id, SETUP_STATUS["ERROR"])
+            # Like an answer, the refusal is only for the writer.
+            return self._answer_endpoints(
+                stream_idx, _setup_endpoints_status(session_id, SETUP_STATUS["ERROR"])
             )
-            return
 
         if not self.streaming_allowed:
             logger.info("[%s] Streaming is turned off, refusing it", session_id)
-            self._management[stream_idx].get_characteristic("SetupEndpoints").set_value(
-                tlv.encode(
-                    SETUP_TYPES["SESSION_ID"],
-                    session_id.bytes,
-                    SETUP_TYPES["STATUS"],
-                    SETUP_STATUS["ERROR"],
-                    to_base64=True,
-                )
+            return self._answer_endpoints(
+                stream_idx, _setup_endpoints_status(session_id, SETUP_STATUS["ERROR"])
             )
-            return
 
         # Configure the SetupEndpoints response
 
@@ -1888,9 +1890,14 @@ def set_endpoints(self, value: str, stream_idx: int | None = None) -> None:
             "audio_backchannel": audio_backchannel,
         }
 
+        return self._answer_endpoints(stream_idx, response_tlv)
+
+    def _answer_endpoints(self, stream_idx: int, answer: str) -> str:
+        """Set the Setup Endpoints answer as the value and return it."""
         self._management[stream_idx].get_characteristic("SetupEndpoints").set_value(
-            response_tlv
+            answer, should_notify=False
         )
+        return answer
 
     async def run(self) -> None:
         """Start the HDS listener when recording transport is available."""

pyhap/characteristic.py

@@ -148,6 +148,13 @@ class Characteristic:
     the current temperature. Characteristics are contained in services.
     Each characteristic has a unique type UUID and a set of properties,
     like format, min and max values, valid values and others.
+
+    Set ``read_back_response`` to True for a readable control point that a
+    controller writes and then reads, such as Setup Endpoints: what the
+    setter returns or leaves as the value is read back only by the connection
+    that wrote, and the shared value stays as it was. An async setter must
+    return the answer and must not set the value itself, a value it sets is
+    shared with every controller.
     """
 
     __slots__ = (
@@ -168,6 +175,7 @@ class Characteristic:
         "_always_null",
         "_setter_addr_cache",
         "_setter_addr_cache_for",
+        "read_back_response",
     )
 
     def __init__(
@@ -224,6 +232,10 @@ def __init__(
         self._loader_display_name: str | None = None
         self._to_hap_cache_with_value: dict[str, Any] | None = None
         self._to_hap_cache: dict[str, Any] | None = None
+        # For a control point a controller writes and then reads, such as
+        # Setup Endpoints: the setter's return value is what the writing
+        # connection reads back, like a write response.
+        self.read_back_response = False
 
     @property
     def display_name(self) -> str | None:

tests/test_accessory_driver.py

@@ -2928,6 +2928,113 @@ def test_prepare_bounds_prepared_writes(driver: AccessoryDriver) -> None:
     assert MAX_PREPARED_WRITES_PER_CLIENT + 4 in pending
 
 
+def _add_read_back_char(driver: AccessoryDriver) -> tuple[Accessory, Characteristic]:
+    """Add a pr and pw char that answers a write with 7 to read back."""
+    acc, char, _ = _add_bridged_or_standalone(driver, ["pr", "pw"], False)
+    char.getter_callback = None
+    char.setter_callback = MagicMock(return_value=7)
+    char.read_back_response = True
+    return acc, char
+
+
+@pytest.mark.parametrize(
+    ("reader", "expected"),
+    [
+        pytest.param(WRITER, 7, id="writer"),
+        pytest.param(OTHER_CONTROLLER, 0, id="other-controller"),
+    ],
+)
+def test_read_back_response_only_for_writer(
+    driver: AccessoryDriver, reader: Any, expected: int
+) -> None:
+    """Only the writing controller reads back what the setter answered."""
+    acc, char = _add_read_back_char(driver)
+
+    assert _write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None
+
+    assert _read(driver, f"{acc.aid}.{acc.iid_manager.get_iid(char)}", reader) == (
+        expected
+    )
+    acc_rep = driver.get_accessories()[HAP_REPR_ACCS][-1]
+    assert acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0][HAP_REPR_VALUE] == 0
+
+
+def test_read_back_prefers_the_returned_answer(driver: AccessoryDriver) -> None:
+    """The answer a setter returns wins over a value it sets itself."""
+    acc, char = _add_read_back_char(driver)
+
+    def _setter(value: int) -> int:
+        char.set_value(42)
+        return 7
+
+    char.setter_callback = _setter
+
+    assert _write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None
+
+    char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}"
+    assert _read(driver, char_id, WRITER) == 7
+    assert _read(driver, char_id, OTHER_CONTROLLER) == 0
+
+
+@pytest.mark.parametrize(
+    ("reader", "expected"),
+    [
+        pytest.param(WRITER, 42, id="writer"),
+        pytest.param(OTHER_CONTROLLER, 0, id="other-controller"),
+    ],
+)
+def test_read_back_value_set_by_setter_only_for_writer(
+    driver: AccessoryDriver, reader: Any, expected: int
+) -> None:
+    """A value the setter sets itself on a read back char stays with the writer."""
+    acc, char = _add_read_back_char(driver)
+
+    def _setter(value: int) -> None:
+        char.set_value(42)
+
+    char.setter_callback = _setter
+
+    assert _write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None
+
+    assert _read(driver, f"{acc.aid}.{acc.iid_manager.get_iid(char)}", reader) == (
+        expected
+    )
+    acc_rep = driver.get_accessories()[HAP_REPR_ACCS][-1]
+    assert acc_rep[HAP_REPR_SERVICES][-1][HAP_REPR_CHARS][0][HAP_REPR_VALUE] == 0
+
+
+async def test_async_read_back_response(driver: AccessoryDriver) -> None:
+    """A coroutine setter's answer is read back, not sent as a write response."""
+    acc, char = _add_read_back_char(driver)
+    char.setter_callback = _async_return_one
+
+    assert await _controller_write(driver, acc, char, **{HAP_REPR_VALUE: 5}) is None
+
+    char_id = f"{acc.aid}.{acc.iid_manager.get_iid(char)}"
+    assert _read(driver, char_id, WRITER) == 1
+    assert _read(driver, char_id, OTHER_CONTROLLER) == 0
+
+
+@pytest.mark.parametrize(
+    ("value", "setter"),
+    [
+        pytest.param("notanint", MagicMock(return_value=None), id="invalid-value"),
+        pytest.param(5, MagicMock(return_value=None), id="setter-does-nothing"),
+    ],
+)
+def test_read_back_without_answer_keeps_nothing_for_writer(
+    driver: AccessoryDriver, value: Any, setter: MagicMock
+) -> None:
+    """Neither the former value nor the request is kept as the writer's answer."""
+    acc, char = _add_read_back_char(driver)
+    char.setter_callback = setter
+
+    _write(driver, acc, char, **{HAP_REPR_VALUE: value})
+    char.set_value(3)
+
+    assert _read(driver, f"{acc.aid}.{acc.iid_manager.get_iid(char)}", WRITER) == 3
+
+
 CLIENT = ("1.2.3.4", 5)