Code Cleanup (#158)

* print coverage report

* create sonar-project property file

* install all py dependencies in one step

* code cleanup

* reduce cognitive complexity

* do not build on *.yml changes

.github/workflows/python-app.yml

@@ -38,7 +38,7 @@ jobs:
         timezoneWindows: "Europe/Berlin"
     - uses: actions/checkout@v4
       with:
-        fetch-depth: 0           # Fetch all history for all tags and branches
+        fetch-depth: 0  # Shallow clones should be disabled for a better relevancy of analysis
     - name: Set up Python 3.12
       uses: actions/setup-python@v5
       with:
@@ -46,7 +46,7 @@ jobs:
     - name: Install dependencies
       run: |
         python -m pip install --upgrade pip
-        pip install flake8 pytest pytest-asyncio
+        pip install flake8 pytest pytest-asyncio pytest-cov coverage
         if [ -f requirements.txt ]; then pip install -r requirements.txt; fi
     - name: Lint with flake8
       run: |
@@ -56,19 +56,13 @@ jobs:
         flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
     - name: Test with pytest
       run: |
-        pip install pytest pytest-cov
         #pytest app --doctest-modules --junitxml=junit/test-results.xml --cov=com --cov-report=xml --cov-report=html
         python -m pytest app --cov=app/src --cov-report=xml
+        coverage report
     - name: Analyze with SonarCloud
       uses: SonarSource/sonarcloud-github-action@v2.2.0
       env:
+        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}  # Needed to get PR information, if any
         SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
       with:
         projectBaseDir: .
-        args:
-          -Dsonar.projectKey=s-allius_tsun-gen3-proxy
-          -Dsonar.organization=s-allius
-          -Dsonar.python.version=3.12
-          -Dsonar.python.coverage.reportPaths=coverage.xml
-          -Dsonar.tests=system_tests,app/tests
-          -Dsonar.source=app/src

app/src/gen3/talent.py

@@ -1,6 +1,6 @@
 import struct
 import logging
-import pytz
+from zoneinfo import ZoneInfo
 from datetime import datetime
 from tzlocal import get_localzone
 
@@ -42,6 +42,7 @@ class Control:
 class Talent(Message):
     MB_START_TIMEOUT = 40
     MB_REGULAR_TIMEOUT = 60
+    TXT_UNKNOWN_CTRL = 'Unknown Ctrl'
 
     def __init__(self, server_side: bool, id_str=b''):
         super().__init__(server_side, self.send_modbus_cb, mb_timeout=15)
@@ -75,7 +76,7 @@ class Talent(Message):
         self.node_id = 'G3'     # will be overwritten in __set_serial_no
         self.mb_timer = Timer(self.mb_timout_cb, self.node_id)
         self.mb_timeout = self.MB_REGULAR_TIMEOUT
-        self.mb_start_timeout = self.MB_START_TIMEOUT
+        self.mb_first_timeout = self.MB_START_TIMEOUT
         self.modbus_polling = False
 
     '''
@@ -246,7 +247,7 @@ class Talent(Message):
 
     def _utcfromts(self, ts: float):
         '''converts inverter timestamp into unix time (epoche)'''
-        dt = datetime.fromtimestamp(ts/1000, pytz.UTC). \
+        dt = datetime.fromtimestamp(ts/1000, tz=ZoneInfo("UTC")). \
             replace(tzinfo=get_localzone())
         return dt.timestamp()
 
@@ -354,7 +355,7 @@ class Talent(Message):
             else:
                 self.forward()
         else:
-            logger.warning('Unknown Ctrl')
+            logger.warning(self.TXT_UNKNOWN_CTRL)
             self.inc_counter('Unknown_Ctrl')
             self.forward()
 
@@ -397,7 +398,7 @@ class Talent(Message):
                              f'  offset: {self.ts_offset}')
                 return  # ignore received response
         else:
-            logger.warning('Unknown Ctrl')
+            logger.warning(self.TXT_UNKNOWN_CTRL)
             self.inc_counter('Unknown_Ctrl')
 
         self.forward()
@@ -431,7 +432,7 @@ class Talent(Message):
         elif self.ctrl.is_resp():
             return  # ignore received response
         else:
-            logger.warning('Unknown Ctrl')
+            logger.warning(self.TXT_UNKNOWN_CTRL)
             self.inc_counter('Unknown_Ctrl')
 
         self.forward()
@@ -444,14 +445,14 @@ class Talent(Message):
             self.__process_data()
             self.state = State.up  # allow MODBUS cmds
             if (self.modbus_polling):
-                self.mb_timer.start(self.mb_start_timeout)
+                self.mb_timer.start(self.mb_first_timeout)
                 self.db.set_db_def_value(Register.POLLING_INTERVAL,
                                          self.mb_timeout)
 
         elif self.ctrl.is_resp():
             return  # ignore received response
         else:
-            logger.warning('Unknown Ctrl')
+            logger.warning(self.TXT_UNKNOWN_CTRL)
             self.inc_counter('Unknown_Ctrl')
 
         self.forward()
@@ -471,7 +472,7 @@ class Talent(Message):
         elif self.ctrl.is_ind():
             pass  # Ok, nothing to do
         else:
-            logger.warning('Unknown Ctrl')
+            logger.warning(self.TXT_UNKNOWN_CTRL)
             self.inc_counter('Unknown_Ctrl')
         self.forward()
 
@@ -519,7 +520,7 @@ class Talent(Message):
                     self.new_data[key] = True
                 self.modbus_elms += 1          # count for unit tests
         else:
-            logger.warning('Unknown Ctrl')
+            logger.warning(self.TXT_UNKNOWN_CTRL)
             self.inc_counter('Unknown_Ctrl')
             self.forward()
 

app/src/gen3plus/solarman_v5.py

@@ -57,6 +57,8 @@ class SolarmanV5(Message):
     '''regular Modbus polling time in server mode'''
     MB_CLIENT_DATA_UP = 30
     '''Data up time in client mode'''
+    HDR_FMT = '<BLLL'
+    '''format string for packing of the header'''
 
     def __init__(self, server_side: bool, client_mode: bool):
         super().__init__(server_side, self.send_modbus_cb, mb_timeout=5)
@@ -133,7 +135,7 @@ class SolarmanV5(Message):
         self.node_id = 'G3P'  # will be overwritten in __set_serial_no
         self.mb_timer = Timer(self.mb_timout_cb, self.node_id)
         self.mb_timeout = self.MB_REGULAR_TIMEOUT
-        self.mb_start_timeout = self.MB_START_TIMEOUT
+        self.mb_first_timeout = self.MB_START_TIMEOUT
         '''timer value for next Modbus polling request'''
         self.modbus_polling = False
 
@@ -180,7 +182,7 @@ class SolarmanV5(Message):
         if self.state is not State.up:
             self.state = State.up
             if (self.modbus_polling):
-                self.mb_timer.start(self.mb_start_timeout)
+                self.mb_timer.start(self.mb_first_timeout)
                 self.db.set_db_def_value(Register.POLLING_INTERVAL,
                                          self.mb_timeout)
 
@@ -225,23 +227,25 @@ class SolarmanV5(Message):
 
             if self.header_valid and len(self._recv_buffer) >= \
                (self.header_len + self.data_len+2):
-                log_lvl = self.log_lvl.get(self.control, logging.WARNING)
+                self.__process_complete_received_msg()
-                if callable(log_lvl):
-                    log_lvl = log_lvl()
-                hex_dump_memory(log_lvl, f'Received from {self.addr}:',
-                                self._recv_buffer, self.header_len +
-                                self.data_len+2)
-                if self.__trailer_is_ok(self._recv_buffer, self.header_len
-                                        + self.data_len + 2):
-                    if self.state == State.init:
-                        self.state = State.received
-
-                    self.__set_serial_no(self.snr)
-                    self.__dispatch_msg()
                 self.__flush_recv_msg()
             else:
                 return 0  # wait 0s before sending a response
 
+    def __process_complete_received_msg(self):
+        log_lvl = self.log_lvl.get(self.control, logging.WARNING)
+        if callable(log_lvl):
+            log_lvl = log_lvl()
+        hex_dump_memory(log_lvl, f'Received from {self.addr}:',
+                        self._recv_buffer, self.header_len +
+                        self.data_len+2)
+        if self.__trailer_is_ok(self._recv_buffer, self.header_len
+                                + self.data_len + 2):
+            if self.state == State.init:
+                self.state = State.received
+            self.__set_serial_no(self.snr)
+            self.__dispatch_msg()
+
     def forward(self, buffer, buflen) -> None:
         '''add the actual receive msg to the forwarding queue'''
         if self.no_forwarding:
@@ -500,7 +504,7 @@ class SolarmanV5(Message):
 
     def msg_dev_ind(self):
         data = self._recv_buffer[self.header_len:]
-        result = struct.unpack_from('<BLLL', data, 0)
+        result = struct.unpack_from(self.HDR_FMT, data, 0)
         ftype = result[0]  # always 2
         total = result[1]
         tim = result[2]
@@ -543,7 +547,7 @@ class SolarmanV5(Message):
 
     def msg_sync_start(self):
         data = self._recv_buffer[self.header_len:]
-        result = struct.unpack_from('<BLLL', data, 0)
+        result = struct.unpack_from(self.HDR_FMT, data, 0)
         ftype = result[0]
         total = result[1]
         self.time_ofs = result[3]
@@ -608,28 +612,30 @@ class SolarmanV5(Message):
                 self.publish_mqtt(f'{self.entity_prfx}{node_id}{key}', data_json)  # noqa: E501
                 return
         elif ftype == self.MB_RTU_CMD:
-            valid = data[1]
+            self.__modbus_command_rsp(data)
-            modbus_msg_len = self.data_len - 14
-            # logger.debug(f'modbus_len:{modbus_msg_len} accepted:{valid}')
-            if valid == 1 and modbus_msg_len > 4:
-                # logger.info(f'first byte modbus:{data[14]}')
-                inv_update = False
-                self.modbus_elms = 0
-
-                for key, update, _ in self.mb.recv_resp(self.db, data[14:],
-                                                        self.node_id):
-                    self.modbus_elms += 1
-                    if update:
-                        if key == 'inverter':
-                            inv_update = True
-                        self._set_mqtt_timestamp(key, self._timestamp())
-                        self.new_data[key] = True
-
-                if inv_update:
-                    self.__build_model_name()
             return
         self.__forward_msg()
 
+    def __modbus_command_rsp(self, data):
+        '''precess MODBUS RTU response'''
+        valid = data[1]
+        modbus_msg_len = self.data_len - 14
+        # logger.debug(f'modbus_len:{modbus_msg_len} accepted:{valid}')
+        if valid == 1 and modbus_msg_len > 4:
+            # logger.info(f'first byte modbus:{data[14]}')
+            inv_update = False
+            self.modbus_elms = 0
+            for key, update, _ in self.mb.recv_resp(self.db, data[14:],
+                                                    self.node_id):
+                self.modbus_elms += 1
+                if update:
+                    if key == 'inverter':
+                        inv_update = True
+                    self._set_mqtt_timestamp(key, self._timestamp())
+                    self.new_data[key] = True
+            if inv_update:
+                self.__build_model_name()
+
     def msg_hbeat_ind(self):
         data = self._recv_buffer[self.header_len:]
         result = struct.unpack_from('<B', data, 0)
@@ -641,7 +647,7 @@ class SolarmanV5(Message):
 
     def msg_sync_end(self):
         data = self._recv_buffer[self.header_len:]
-        result = struct.unpack_from('<BLLL', data, 0)
+        result = struct.unpack_from(self.HDR_FMT, data, 0)
         ftype = result[0]
         total = result[1]
         self.time_ofs = result[3]

app/src/messages.py

@@ -14,6 +14,25 @@ else:  # pragma: no cover
 logger = logging.getLogger('msg')
 
 
+def __hex_val(n, data, data_len):
+    line = ''
+    for j in range(n-16, n):
+        if j >= data_len:
+            break
+        line += '%02x ' % abs(data[j])
+    return line
+
+
+def __asc_val(n, data, data_len):
+    line = ''
+    for j in range(n-16, n):
+        if j >= data_len:
+            break
+        c = data[j] if not (data[j] < 0x20 or data[j] > 0x7e) else '.'
+        line += '%c' % c
+    return line
+
+
 def hex_dump_memory(level, info, data, data_len):
     n = 0
     lines = []
@@ -26,20 +45,9 @@ def hex_dump_memory(level, info, data, data_len):
         line = '  '
         line += '%04x | ' % (i)
         n += 16
-
+        line += __hex_val(n, data, data_len)
-        for j in range(n-16, n):
-            if j >= data_len:
-                break
-            line += '%02x ' % abs(data[j])
-
         line += ' ' * (3 * 16 + 9 - len(line)) + ' | '
-
+        line += __asc_val(n, data, data_len)
-        for j in range(n-16, n):
-            if j >= data_len:
-                break
-            c = data[j] if not (data[j] < 0x20 or data[j] > 0x7e) else '.'
-            line += '%c' % c
-
         lines.append(line)
 
     tracer.log(level, '\n'.join(lines))

app/tests/test_solarman.py

@@ -41,7 +41,7 @@ class MemoryStream(SolarmanV5):
         super().__init__(server_side, client_mode=False)
         if server_side:
             self.mb.timeout = 0.4   # overwrite for faster testing
-        self.mb_start_timeout = 0.5
+        self.mb_first_timeout = 0.5
         self.mb_timeout = 0.5
         self.writer = Writer()
         self.mqtt = Mqtt()
@@ -1692,7 +1692,7 @@ async def test_start_client_mode(config_tsun_inv1):
     assert m.no_forwarding == False
     assert m.mb_timer.tim == None
     assert asyncio.get_running_loop() == m.mb_timer.loop
-    await m.send_start_cmd(get_sn_int(), '192.168.1.1', m.mb_start_timeout)
+    await m.send_start_cmd(get_sn_int(), '192.168.1.1', m.mb_first_timeout)
     assert m.writer.sent_pdu==bytearray(b'\xa5\x17\x00\x10E\x01\x00!Ce{\x02\xb0\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x030\x00\x000J\xde\xf1\x15')
     assert m.db.get_db_value(Register.IP_ADDRESS) == '192.168.1.1'
     assert m.db.get_db_value(Register.POLLING_INTERVAL) == 0.5

app/tests/test_talent.py

@@ -27,7 +27,7 @@ class MemoryStream(Talent):
         super().__init__(server_side)
         if server_side:
             self.mb.timeout = 0.4   # overwrite for faster testing
-        self.mb_start_timeout = 0.5
+        self.mb_first_timeout = 0.5
         self.mb_timeout = 0.5
         self.writer = Writer()
         self.__msg = msg

sonar-project.properties

@@ -0,0 +1,17 @@
+sonar.projectKey=s-allius_tsun-gen3-proxy
+sonar.organization=s-allius
+
+# This is the name and version displayed in the SonarCloud UI.
+sonar.projectName=tsun-gen3-proxy
+#sonar.projectVersion=1.0
+
+
+# Path is relative to the sonar-project.properties file. Replace "\" by "/" on Windows.
+sonar.sources=app/src
+
+# Encoding of the source code. Default is default system encoding
+#sonar.sourceEncoding=UTF-8
+
+sonar.python.version=3.12
+sonar.python.coverage.reportPaths=coverage.xml
+sonar.tests=system_tests,app/tests