Repository: XKNX/xknx
Pull request #766 (2021-09-30 20:35:19)

DPTString: replace invalid characters with question marks
<!--
  You are awesome! Thanks for contributing to our project!
  Please, DO NOT DELETE ANY TEXT from this template!.
-->
## Description
<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.
-->
DPTString: replace invalid characters with question marks

When trying to send an invalid character (eg. "š") we would raise an error at https://github.com/XKNX/xknx/blob/c19ea3d4347513d6052f358ea98948e248a68e97/xknx/telegram/apci.py#L271 because
```python
>>> ord("š")
353
>>> bytes((353,))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: bytes must be in range(0, 256)
```
and this would crash our outgoing TelegramQueue.

see https://community.home-assistant.io/t/outgoing-knx-telegrams-are-not-send-after-ha-core-update/340444

## Type of change
<!--
Please tick the applicable options.
NOTE: Ticking multiple options most likely indicates
that your change is to big and it is suggested to split it into several smaller PRs.
-->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## Checklist:

- [x] I have performed a self-review of my own code
- [ ] The documentation has been adjusted accordingly
- [x] The changes generate no new warnings
- [x] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog
- [ ] The Homeassistant plugin has been adjusted in case of new config options


codecov[bot]: # [Codecov](https://codecov.io/gh/XKNX/xknx/pull/766?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) Report
> Merging [#766](https://codecov.io/gh/XKNX/xknx/pull/766?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (31111e1) into [main](https://codecov.io/gh/XKNX/xknx/commit/c19ea3d4347513d6052f358ea98948e248a68e97?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (c19ea3d) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/XKNX/xknx/pull/766/graphs/tree.svg?width=650&height=150&src=pr&token=irWbIygS84&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)](https://codecov.io/gh/XKNX/xknx/pull/766?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)

```diff
@@            Coverage Diff             @@
##             main     #766      +/-   ##
==========================================
- Coverage   94.67%   94.67%   -0.01%     
==========================================
  Files         106      106              
  Lines        6663     6661       -2     
==========================================
- Hits         6308     6306       -2     
  Misses        355      355              
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/766?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/dpt/dpt\_string.py](https://codecov.io/gh/XKNX/xknx/pull/766/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9kcHQvZHB0X3N0cmluZy5weQ==) | `100.00% <100.00%> (ø)` | |

farmio: oh come on @codecov you are pulling a coveralls now!
marvin-w: Can be merged after CI is done, conflicts are solved.

Patch:
diff --git a/changelog.md b/changelog.md
index 412b0484..c5a20482 100644
--- a/changelog.md
+++ b/changelog.md
@@ -4,6 +4,7 @@
 
 ### Internals
 
+- DPTString: replace invalid characters with question marks in `to_knx`
 - Catch and log exceptions raised in callbacks to not stall the TelegramQueue
 - Handle callbacks in separate asyncio Tasks
 - GatewayScanFilter: Ignore non-gateway KNX/IP devices
diff --git a/xknx/dpt/dpt_string.py b/xknx/dpt/dpt_string.py
index c43d715f..baee9900 100644
--- a/xknx/dpt/dpt_string.py
+++ b/xknx/dpt/dpt_string.py
@@ -36,11 +36,11 @@ class DPTString(DPTBase):
             knx_value = str(value)
             if not cls._test_boundaries(knx_value):
                 raise ValueError
-            raw = []
-            for character in knx_value:
-                raw.append(ord(character))
+            raw = [ord(character) for character in knx_value]
             raw.extend([0] * (cls.payload_length - len(raw)))
-            return tuple(raw)
+            # replace invalid characters with question marks
+            # bytes(knx_value, 'ascii') would raise UnicodeEncodeError
+            return tuple(map(lambda char: char if char <= 0xFF else ord("?"), raw))
         except ValueError:
             raise ConversionError("Could not serialize %s" % cls.__name__, value=value)
 
diff --git a/xknx/knxip/cemi_frame.py b/xknx/knxip/cemi_frame.py
index f32f2610..ac66875d 100644
--- a/xknx/knxip/cemi_frame.py
+++ b/xknx/knxip/cemi_frame.py
@@ -135,7 +135,8 @@ class CEMIFrame:
     def from_knx_data_link_layer(self, cemi: bytes) -> int:
         """Parse L_DATA_IND, CEMIMessageCode.L_DATA_REQ, CEMIMessageCode.L_DATA_CON."""
         if len(cemi) < 11:
-            # eg. ETS Line-Scan issues L_DATA_IND with length 10
+            # eg. ETS Line-Scan issues L_DATA_IND with length 10 or checking for individual addresses
+            # at start of ETS programming procedure
             raise UnsupportedCEMIMessage(
                 "CEMI too small. Length: {}; CEMI: {}".format(len(cemi), cemi.hex())
             )


Test patch:
diff --git a/test/dpt_tests/dpt_string_test.py b/test/dpt_tests/dpt_string_test.py
index cf3c8bc3..45070e2e 100644
--- a/test/dpt_tests/dpt_string_test.py
+++ b/test/dpt_tests/dpt_string_test.py
@@ -95,6 +95,29 @@ class TestDPTString:
         assert DPTString.to_knx(string) == raw
         assert DPTString.from_knx(raw) == string
 
+    def test_to_knx_invalid_chars(self):
+        """Test streaming string with invalid chars."""
+        raw = (
+            0x4D,
+            0x61,
+            0x74,
+            0x6F,
+            0x75,
+            0x3F,
+            0x00,
+            0x00,
+            0x00,
+            0x00,
+            0x00,
+            0x00,
+            0x00,
+            0x00,
+        )
+        string = "Matouš"
+        knx_string = "Matou?"
+        assert DPTString.to_knx(string) == raw
+        assert DPTString.from_knx(raw) == knx_string
+
     def test_to_knx_too_long(self):
         """Test serializing DPTString to KNX with wrong value (to long)."""
         with pytest.raises(ConversionError):