Repository: kipe/enocean

Issue:
Re-structure Communicator
Based on discussion in 3bc29571ed1aef69f5b03bf1416e7bb15a072b1e

@romor wrote:

> Just my impression: I do not think that it is a nice solution to store a reference to communicator in `UTETeachIn` class. It could be provided as an argument to `send_response()` instead.
> 
> But even this is not nice, IMO. I do not think that a `Packet` class should autonomously send radio packets. I think this should be handled by the `Communicator` class instead.

@kipe wrote:

> Good points, and I agree with you.
> `Communicator` should indeed handle all sending and receiving, however I'm not sure if it's possible to implement without a reference at the moment. As the implementation is currently, the base `Communicator` doesn't implement any kind of loop. This makes it difficult to implement any kind of "packet interception" to the `Communicator` (I'd like all communicators to behave the same).
> 
> One option would be to call something like `Communicator.send_responses()` on each loop in the implementations. This would then check, if there's responses to be sent and sent them.
> 
> Another option would be to re-structure the communicators to implement a `SerialCommunicator.loop()` or something like that, which would take care of the implementation specific behavior, while `Communicator.run()` would implement the main loop. In this implementation, `SerialCommunicator.loop()` would return a list of new packages to be parsed, and `Communicator.run()` would take care of handling them (mainly putting to `receive` -queue), sending a response as needed.


Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/21960354/badge)](https://coveralls.io/builds/21960354)

Coverage increased (+0.01%) to 91.277% when pulling **8099d741756a24325288d6d917af81fa7d15e8d8 on stv0g:issue_34** into **cecf7ddd3de5ba39fc9df4849fcaf110ac31411e on kipe:master**.


Patch:
diff --git a/enocean/communicators/communicator.py b/enocean/communicators/communicator.py
index 4562219..4f9d540 100644
--- a/enocean/communicators/communicator.py
+++ b/enocean/communicators/communicator.py
@@ -8,7 +8,7 @@ try:
     import queue
 except ImportError:
     import Queue as queue
-from enocean.protocol.packet import Packet
+from enocean.protocol.packet import Packet, UTETeachInPacket
 from enocean.protocol.constants import PACKET, PARSE_RESULT, RETURN_CODE
 
 
@@ -61,7 +61,7 @@ class Communicator(threading.Thread):
         ''' Parses messages and puts them to receive queue '''
         # Loop while we get new messages
         while True:
-            status, self._buffer, packet = Packet.parse_msg(self._buffer, communicator=self)
+            status, self._buffer, packet = Packet.parse_msg(self._buffer)
             # If message is incomplete -> break the loop
             if status == PARSE_RESULT.INCOMPLETE:
                 return status
@@ -69,6 +69,12 @@ class Communicator(threading.Thread):
             # If message is OK, add it to receive queue or send to the callback method
             if status == PARSE_RESULT.OK and packet:
                 packet.received = datetime.datetime.now()
+
+                if isinstance(packet, UTETeachInPacket) and self.teach_in:
+                    response_packet = packet.create_response_packet(self.base_id)
+                    self.logger.info('Sending response to UTE teach-in.')
+                    self.send(response_packet)
+
                 if self.__callback is None:
                     self.receive.put(packet)
                 else:
diff --git a/enocean/protocol/packet.py b/enocean/protocol/packet.py
index 8cfa0ef..63e70b0 100644
--- a/enocean/protocol/packet.py
+++ b/enocean/protocol/packet.py
@@ -94,7 +94,7 @@ class Packet(object):
         self.status = enocean.utils.from_bitarray(value)
 
     @staticmethod
-    def parse_msg(buf, communicator=None):
+    def parse_msg(buf):
         '''
         Parses message from buffer.
         returns:
@@ -146,11 +146,7 @@ class Packet(object):
         if packet_type == PACKET.RADIO_ERP1:
             # Need to handle UTE Teach-in here, as it's a separate packet type...
             if data[0] == RORG.UTE:
-                packet = UTETeachIn(packet_type, data, opt_data, communicator=communicator)
-                # Send a response automatically, works only if
-                # - communicator is set
-                # - communicator.teach_in == True
-                packet.send_response()
+                packet = UTETeachInPacket(packet_type, data, opt_data)
             else:
                 packet = RadioPacket(packet_type, data, opt_data)
         elif packet_type == PACKET.RESPONSE:
@@ -348,7 +344,7 @@ class RadioPacket(Packet):
         return super(RadioPacket, self).parse()
 
 
-class UTETeachIn(RadioPacket):
+class UTETeachInPacket(RadioPacket):
     # Request types
     TEACH_IN = 0b00
     DELETE = 0b01
@@ -369,10 +365,6 @@ class UTETeachIn(RadioPacket):
 
     contains_eep = True
 
-    def __init__(self, packet_type, data=None, optional=None, communicator=None):
-        self.__communicator = communicator
-        super(UTETeachIn, self).__init__(packet_type=packet_type, data=data, optional=optional)
-
     @property
     def bidirectional(self):
         return not self.unidirectional
@@ -386,7 +378,7 @@ class UTETeachIn(RadioPacket):
         return self.request_type == self.DELETE
 
     def parse(self):
-        super(UTETeachIn, self).parse()
+        super(UTETeachInPacket, self).parse()
         self.unidirectional = not self._bit_data[DB6.BIT_7]
         self.response_expected = not self._bit_data[DB6.BIT_6]
         self.request_type = enocean.utils.from_bitarray(self._bit_data[DB6.BIT_5:DB6.BIT_3])
@@ -399,7 +391,7 @@ class UTETeachIn(RadioPacket):
             self.learn = True
         return self.parsed
 
-    def _create_response_packet(self, sender_id, response=TEACHIN_ACCEPTED):
+    def create_response_packet(self, sender_id, response=TEACHIN_ACCEPTED):
         # Create data:
         # - Respond with same RORG (UTE Teach-in)
         # - Always use bidirectional communication, set response code, set command identifier.
@@ -415,17 +407,6 @@ class UTETeachIn(RadioPacket):
 
         return RadioPacket(PACKET.RADIO_ERP1, data=data, optional=optional)
 
-    def send_response(self, response=TEACHIN_ACCEPTED):
-        if self.__communicator is None:
-            self.logger.error('Communicator not set, cannot send UTE teach-in response.')
-            return
-        if not self.__communicator.teach_in:
-            self.logger.info('Communicator not set to teach-in mode, not sending UTE teach-in response.')
-            return
-        self.logger.info('Sending response to UTE teach-in.')
-        self.__communicator.send(
-            self._create_response_packet(self.__communicator.base_id))
-
 
 class ResponsePacket(Packet):
     response = 0
diff --git a/examples/example_D2-05-00.py b/examples/example_D2-05-00.py
index bb03566..20f655d 100644
--- a/examples/example_D2-05-00.py
+++ b/examples/example_D2-05-00.py
@@ -12,7 +12,7 @@ import time
 import traceback
 import enocean.utils
 from enocean.communicators import SerialCommunicator
-from enocean.protocol.packet import RadioPacket, UTETeachIn
+from enocean.protocol.packet import RadioPacket, UTETeachInPacket
 from enocean.protocol.constants import RORG
 
 try:
@@ -47,7 +47,7 @@ while communicator.is_alive():
     try:
         # Loop to empty the queue...
         packet = communicator.receive.get(block=True, timeout=1)
-        if isinstance(packet, UTETeachIn):
+        if isinstance(packet, UTETeachInPacket):
             print('New device learned! The ID is %s.' % (packet.sender_hex))
             devices_learned.append(packet.sender)
     except queue.Empty:
diff --git a/examples/example_DO21-11B-E.py b/examples/example_DO21-11B-E.py
index c828ca4..52464ec 100644
--- a/examples/example_DO21-11B-E.py
+++ b/examples/example_DO21-11B-E.py
@@ -12,7 +12,7 @@ import time
 import traceback
 import enocean.utils
 from enocean.communicators import SerialCommunicator
-from enocean.protocol.packet import RadioPacket, UTETeachIn
+from enocean.protocol.packet import RadioPacket, UTETeachInPacket
 from enocean.protocol.constants import RORG
 
 try:
@@ -60,7 +60,7 @@ while communicator.is_alive():
     try:
         # Loop to empty the queue...
         packet = communicator.receive.get(block=True, timeout=1)
-        if isinstance(packet, UTETeachIn):
+        if isinstance(packet, UTETeachInPacket):
             print('New device learned! The ID is %s.' % (packet.sender_hex))
             devices_learned.append(packet.sender)
     except queue.Empty:


Test patch:
diff --git a/enocean/protocol/tests/test_teachin.py b/enocean/protocol/tests/test_teachin.py
index 002b4c0..d908a94 100644
--- a/enocean/protocol/tests/test_teachin.py
+++ b/enocean/protocol/tests/test_teachin.py
@@ -20,8 +20,7 @@ def test_ute_in():
             0xD4, 0xA0, 0xFF, 0x3E, 0x00, 0x01, 0x01, 0xD2, 0x01, 0x94, 0xE3, 0xB9, 0x00,
             0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x40, 0x00,
             0xAB
-        ]),
-        communicator=communicator
+        ])
     )
 
     assert packet.sender_hex == '01:94:E3:B9'
@@ -38,7 +37,7 @@ def test_ute_in():
     assert packet.learn is True
     assert packet.contains_eep is True
 
-    response_packet = packet._create_response_packet(communicator.base_id)
+    response_packet = packet.create_response_packet(communicator.base_id)
     assert response_packet.sender_hex == 'DE:AD:BE:EF'
     assert response_packet.destination_hex == '01:94:E3:B9'
     assert response_packet._bit_data[DB6.BIT_5:DB6.BIT_3] == [False, True]