Merge pull request #250 from ikalchev/v2.8.4

V2.8.4
ikalchev · May 12, 2020 · 9875080 · 9875080
2 parents 9385a07 + 085f0b4
commit 9875080
Show file tree

Hide file tree

Showing 6 changed files with 111 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,13 @@ Sections
 ### Developers
 -->
 
+## [2.8.4] - 2020-05-12
+
+### Fixed
+- Fix race condition that causes pairing and unpairing failures. [#246](https://github.com/ikalchev/HAP-python/pull/246)
+- Fix loop on dropped connections that causes temporary stalls and connection loss. [#249](https://github.com/ikalchev/HAP-python/pull/249)
+- Fix exception on missing video fields. [#245](https://github.com/ikalchev/HAP-python/pull/245)
+
 ## [2.8.3] - 2020-05-01
 
 ### Fixed

diff --git a/pyhap/accessory_driver.py b/pyhap/accessory_driver.py
@@ -509,8 +509,7 @@ def load(self):
     def pair(self, client_uuid, client_public):
         """Called when a client has paired with the accessory.
 
-        Updates the accessory with the paired client and updates the mDNS service. Also,
-        persist the new state.
+        Persist the new accessory state.
 
         :param client_uuid: The client uuid.
         :type client_uuid: uuid.UUID
@@ -527,24 +526,37 @@ def pair(self, client_uuid, client_public):
         logger.info("Paired with %s.", client_uuid)
         self.state.add_paired_client(client_uuid, client_public)
         self.persist()
-        # Safe mode added to avoid error during pairing, see
-        # https://github.com/home-assistant/home-assistant/issues/14567
-        if not self.safe_mode:
-            self.update_advertisement()
         return True
 
     def unpair(self, client_uuid):
         """Removes the paired client from the accessory.
 
-        Updates the accessory and updates the mDNS service. Persist the new accessory
-        state.
+        Persist the new accessory state.
 
         :param client_uuid: The client uuid.
         :type client_uuid: uuid.UUID
         """
         logger.info("Unpairing client %s.", client_uuid)
         self.state.remove_paired_client(client_uuid)
         self.persist()
+
+    def finish_pair(self):
+        """Finishing pairing or unpairing.
+
+        Updates the accessory and updates the mDNS service.
+
+        The mDNS announcement must not be updated until AFTER
+        the final pairing response is sent or homekit will
+        see that the accessory is already paired and assume
+        it should stop pairing.
+        """
+        # Safe mode added to avoid error during pairing, see
+        # https://github.com/home-assistant/home-assistant/issues/14567
+        #
+        # This may no longer be needed now that we defer
+        # updating the advertisement until after the final
+        # pairing response is sent.
+        #
         if not self.safe_mode:
             self.update_advertisement()
 

diff --git a/pyhap/camera.py b/pyhap/camera.py
@@ -223,6 +223,8 @@
 )
 '''Template for the ffmpeg command.'''
 
+logger = logging.getLogger(__name__)
+
 
 class Camera(Accessory):
     """An Accessory that can negotiated camera stream settings with iOS and start a
@@ -310,7 +312,7 @@ def get_supported_audio_stream_config(audio_params):
                 codec = AUDIO_CODEC_TYPES['AACELD']
                 bitrate = AUDIO_CODEC_PARAM_BIT_RATE_TYPES['VARIABLE']
             else:
-                logging.warning('Unsupported codec %s', param_type)
+                logger.warning('Unsupported codec %s', param_type)
                 continue
 
             param_samplerate = codec_param['samplerate']
@@ -321,7 +323,7 @@ def get_supported_audio_stream_config(audio_params):
             elif param_samplerate == 24:
                 samplerate = AUDIO_CODEC_PARAM_SAMPLE_RATE_TYPES['KHZ_24']
             else:
-                logging.warning('Unsupported sample rate %s', param_samplerate)
+                logger.warning('Unsupported sample rate %s', param_samplerate)
                 continue
 
             param_tlv = tlv.encode(AUDIO_CODEC_PARAM_TYPES['CHANNEL'], b'\x01',
@@ -332,7 +334,7 @@ def get_supported_audio_stream_config(audio_params):
             configs += tlv.encode(SUPPORTED_AUDIO_CODECS_TAG, config_tlv)
 
         if not has_supported_codec:
-            logging.warning('Client does not support any audio codec that iOS supports.')
+            logger.warning('Client does not support any audio codec that iOS supports.')
 
             codec = AUDIO_CODEC_TYPES['OPUS']
             bitrate = AUDIO_CODEC_PARAM_BIT_RATE_TYPES['VARIABLE']
@@ -483,20 +485,25 @@ async def _start_stream(self, objs, reconfigure):  # pylint: disable=unused-argu
             video_rtp_param = video_objs.get(VIDEO_TYPES['RTP_PARAM'])
             if video_rtp_param:
                 video_rtp_param_objs = tlv.decode(video_rtp_param)
-                # TODO: Optionals, handle the case where they are missing
-                opts['v_ssrc'] = struct.unpack('<I',
-                    video_rtp_param_objs.get(
-                        RTP_PARAM_TYPES['SYNCHRONIZATION_SOURCE']))[0]
-                opts['v_payload_type'] = \
-                    video_rtp_param_objs.get(RTP_PARAM_TYPES['PAYLOAD_TYPE'])
-                opts['v_max_bitrate'] = struct.unpack('<H',
-                    video_rtp_param_objs.get(RTP_PARAM_TYPES['MAX_BIT_RATE']))[0]
-                opts['v_rtcp_interval'] = struct.unpack('<f',
-                    video_rtp_param_objs.get(RTP_PARAM_TYPES['RTCP_SEND_INTERVAL']))[0]
-                opts['v_max_mtu'] = video_rtp_param_objs.get(RTP_PARAM_TYPES['MAX_MTU'])
+                if RTP_PARAM_TYPES['SYNCHRONIZATION_SOURCE'] in video_rtp_param_objs:
+                    opts['v_ssrc'] = struct.unpack('<I',
+                        video_rtp_param_objs.get(
+                            RTP_PARAM_TYPES['SYNCHRONIZATION_SOURCE']))[0]
+                if RTP_PARAM_TYPES['PAYLOAD_TYPE'] in video_rtp_param_objs:
+                    opts['v_payload_type'] = \
+                        video_rtp_param_objs.get(RTP_PARAM_TYPES['PAYLOAD_TYPE'])
+                if RTP_PARAM_TYPES['MAX_BIT_RATE'] in video_rtp_param_objs:
+                    opts['v_max_bitrate'] = struct.unpack('<H',
+                        video_rtp_param_objs.get(RTP_PARAM_TYPES['MAX_BIT_RATE']))[0]
+                if RTP_PARAM_TYPES['RTCP_SEND_INTERVAL'] in video_rtp_param_objs:
+                    opts['v_rtcp_interval'] = struct.unpack('<f',
+                        video_rtp_param_objs.get(RTP_PARAM_TYPES['RTCP_SEND_INTERVAL']))[0]
+                if RTP_PARAM_TYPES['MAX_MTU'] in video_rtp_param_objs:
+                    opts['v_max_mtu'] = video_rtp_param_objs.get(RTP_PARAM_TYPES['MAX_MTU'])
 
         if audio_tlv:
             audio_objs = tlv.decode(audio_tlv)
+
             opts['a_codec'] = audio_objs[AUDIO_TYPES['CODEC']]
             audio_codec_param_objs = tlv.decode(
                                         audio_objs[AUDIO_TYPES['CODEC_PARAM']])
@@ -534,8 +541,10 @@ async def _start_stream(self, objs, reconfigure):  # pylint: disable=unused-argu
         if success:
             self.streaming_status = STREAMING_STATUS['STREAMING']
         else:
-            logging.error('[%s] Faled to start/reconfigure stream, deleting session.',
-                          session_id)
+            logger.error(
+                '[%s] Failed to start/reconfigure stream, deleting session.',
+                session_id
+            )
             del self.sessions[session_id]
             self.streaming_status = STREAMING_STATUS['AVAILABLE']
 
@@ -560,8 +569,11 @@ async def _stop_stream(self, objs):
         session_info = self.sessions.get(session_id)
 
         if not session_info:
-            logging.error('Requested to stop stream for session %s, but no '
-                          'such session was found', session_id)
+            logger.error(
+                'Requested to stop stream for session %s, but no '
+                'such session was found',
+                session_id
+            )
             return
 
         await self.stop_stream(session_info)
@@ -580,25 +592,25 @@ def set_selected_stream_configuration(self, value):
         :param value: base64-encoded selected configuration in TLV format
         :type value: ``str``
         """
-        logging.debug('set_selected_stream_config - value - %s', value)
+        logger.debug('set_selected_stream_config - value - %s', value)
 
         objs = tlv.decode(value, from_base64=True)
         if SELECTED_STREAM_CONFIGURATION_TYPES['SESSION'] not in objs:
-            logging.error('Bad request to set selected stream configuration.')
+            logger.error('Bad request to set selected stream configuration.')
             return
 
         session = tlv.decode(objs[SELECTED_STREAM_CONFIGURATION_TYPES['SESSION']])
 
         request_type = session[b'\x02'][0]
-        logging.debug('Set stream config request: %d', request_type)
+        logger.debug('Set stream config request: %d', request_type)
         if request_type == 1:
             job = functools.partial(self._start_stream, reconfigure=False)
         elif request_type == 0:
             job = self._stop_stream
         elif request_type == 4:
             job = functools.partial(self._start_stream, reconfigure=True)
         else:
-            logging.error('Unknown request type %d', request_type)
+            logger.error('Unknown request type %d', request_type)
             return
 
         self.driver.add_job(job, objs)
@@ -640,16 +652,18 @@ def set_endpoints(self, value):
         audio_master_key = audio_info_objs[SETUP_SRTP_PARAM['MASTER_KEY']]
         audio_master_salt = audio_info_objs[SETUP_SRTP_PARAM['MASTER_SALT']]
 
-        logging.debug('Received endpoint configuration:'
-                      '\nsession_id: %s\naddress: %s\nis_ipv6: %s'
-                      '\ntarget_video_port: %s\ntarget_audio_port: %s'
-                      '\nvideo_crypto_suite: %s\nvideo_srtp: %s'
-                      '\naudio_crypto_suite: %s\naudio_srtp: %s',
-                      session_id, address, is_ipv6, target_video_port, target_audio_port,
-                      video_crypto_suite,
-                      to_base64_str(video_master_key + video_master_salt),
-                      audio_crypto_suite,
-                      to_base64_str(audio_master_key + audio_master_salt))
+        logger.debug(
+            'Received endpoint configuration:'
+            '\nsession_id: %s\naddress: %s\nis_ipv6: %s'
+            '\ntarget_video_port: %s\ntarget_audio_port: %s'
+            '\nvideo_crypto_suite: %s\nvideo_srtp: %s'
+            '\naudio_crypto_suite: %s\naudio_srtp: %s',
+            session_id, address, is_ipv6, target_video_port, target_audio_port,
+            video_crypto_suite,
+            to_base64_str(video_master_key + video_master_salt),
+            audio_crypto_suite,
+            to_base64_str(audio_master_key + audio_master_salt)
+        )
 
         # Configure the SetupEndpoints response
 
@@ -765,24 +779,30 @@ async def start_stream(self, session_info, stream_config):
         :return: True if and only if starting the stream command was successful.
         :rtype: ``bool``
         """
-        logging.debug('[%s] Starting stream with the following parameters: %s',
-                      session_info['id'], stream_config)
+        logger.debug(
+            '[%s] Starting stream with the following parameters: %s',
+            session_info['id'],
+            stream_config
+        )
 
         cmd = self.start_stream_cmd.format(**stream_config).split()
-        logging.debug('Executing start stream command: "%s"', ' '.join(cmd))
+        logger.debug('Executing start stream command: "%s"', ' '.join(cmd))
         try:
             process = await asyncio.create_subprocess_exec(*cmd,
                     stdout=asyncio.subprocess.DEVNULL,
                     stderr=asyncio.subprocess.PIPE,
                     limit=1024)
         except Exception as e:  # pylint: disable=broad-except
-            logging.error('Failed to start streaming process because of error: %s', e)
+            logger.error('Failed to start streaming process because of error: %s', e)
             return False
 
         session_info['process'] = process
 
-        logging.info('[%s] Started stream process - PID %d',
-                     session_info['id'], process.pid)
+        logger.info(
+            '[%s] Started stream process - PID %d',
+            session_info['id'],
+            process.pid
+        )
 
         return True
 
@@ -800,20 +820,22 @@ async def stop_stream(self, session_info):  # pylint: disable=no-self-use
         session_id = session_info['id']
         ffmpeg_process = session_info.get('process')
         if ffmpeg_process:
-            logging.info('[%s] Stopping stream.', session_id)
+            logger.info('[%s] Stopping stream.', session_id)
             try:
                 ffmpeg_process.terminate()
                 _, stderr = await asyncio.wait_for(
                     ffmpeg_process.communicate(), timeout=2.0)
-                logging.debug('Stream command stderr: %s', stderr)
+                logger.debug('Stream command stderr: %s', stderr)
             except asyncio.TimeoutError:
-                logging.error('Timeout while waiting for the stream process '
-                              'to terminate. Trying with kill.')
+                logger.error(
+                    'Timeout while waiting for the stream process '
+                    'to terminate. Trying with kill.'
+                )
                 ffmpeg_process.kill()
                 await ffmpeg_process.wait()
-            logging.debug('Stream process stopped.')
+            logger.debug('Stream process stopped.')
         else:
-            logging.warning('No process for session ID %s', session_id)
+            logger.warning('No process for session ID %s', session_id)
 
     async def reconfigure_stream(self, session_info, stream_config):
         """Reconfigure the stream so that it uses the given ``stream_config``.

diff --git a/pyhap/const.py b/pyhap/const.py
@@ -1,7 +1,7 @@
 """This module contains constants used by other modules."""
 MAJOR_VERSION = 2
 MINOR_VERSION = 8
-PATCH_VERSION = 3
+PATCH_VERSION = 4
 __short_version__ = '{}.{}'.format(MAJOR_VERSION, MINOR_VERSION)
 __version__ = '{}.{}'.format(__short_version__, PATCH_VERSION)
 REQUIRED_PYTHON_VER = (3, 5)

diff --git a/pyhap/hap_server.py b/pyhap/hap_server.py
@@ -598,6 +598,13 @@ def _handle_add_pairing(self, tlv_objects):
         self.send_header("Content-Type", self.PAIRING_RESPONSE_TYPE)
         self.end_response(data)
 
+        # Avoid updating the announcement until
+        # after the response is sent as homekit will
+        # drop the connection and fail to pair if it
+        # sees the accessory is now paired as it doesn't
+        # know that it was the one doing the pairing.
+        self.accessory_handler.finish_pair()
+
     def _handle_remove_pairing(self, tlv_objects):
         """Remove pairing with the client."""
         logger.debug("Removing client pairing.")
@@ -610,6 +617,10 @@ def _handle_remove_pairing(self, tlv_objects):
         self.send_header("Content-Type", self.PAIRING_RESPONSE_TYPE)
         self.end_response(data)
 
+        # Avoid updating the announcement until
+        # after the response is sent.
+        self.accessory_handler.finish_pair()
+
     def handle_resource(self):
         """Get a snapshot from the camera."""
         if not hasattr(self.accessory_handler.accessory, 'get_snapshot'):
@@ -738,7 +749,8 @@ def recv(self, buflen=1042, flags=0):
                     self.LENGTH_LENGTH, socket.MSG_WAITALL
                 )
                 if not block_length_bytes:
-                    return result
+                    # Connection likely dropped
+                    return b""
                 # Init. info about the block we just started.
                 # Note we are setting the total length to block_length + mac length
                 self.curr_in_total = \
@@ -767,6 +779,9 @@ def recv(self, buflen=1042, flags=0):
                     self.in_count += 1
                     self.curr_in_block = None
                     break
+                elif not actual_len:
+                    # Connection likely dropped
+                    return b""
 
         return result
 

diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py
@@ -55,7 +55,7 @@ def test_persist_load():
 
 
 def test_service_callbacks(driver):
-    bridge = Bridge(driver,"mybridge")
+    bridge = Bridge(driver, "mybridge")
     acc = Accessory(driver, 'TestAcc', aid=2)
     acc2 = Accessory(driver, 'TestAcc2', aid=3)