qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
17.47k stars 37.7k forks source link

Fix Layout & RGB Matrix coordinate data type #23940

Closed waffle87 closed 2 weeks ago

waffle87 commented 2 weeks ago

Description

Title

Types of Changes

Checklist

zvecr commented 2 weeks ago

https://github.com/qmk/qmk_firmware/blob/develop/quantum/rgb_matrix/rgb_matrix_types.h#L54

As the backing data type is integer, its probably more "correct" without this change. Though this might be less risk, and something more targeted could go to develop?

As a potential mid way solution, we could keep the previous behaviour on master and extend the parsing code to produce additional errors ( though this only helps with 3/14 of the current cases...)

diff --git a/lib/python/qmk/c_parse.py b/lib/python/qmk/c_parse.py
index 08d23cf5ba..0f479e4126 100644
--- a/lib/python/qmk/c_parse.py
+++ b/lib/python/qmk/c_parse.py
@@ -218,6 +218,7 @@ def _coerce_led_token(_type, value):

 def _validate_led_config(matrix, matrix_rows, matrix_cols, matrix_indexes, position, position_raw, flags):
     # TODO: Improve crude parsing/validation
     if len(matrix) != matrix_rows and len(matrix) != (matrix_rows / 2):
         raise ValueError("Unable to parse g_led_config matrix data")
@@ -230,6 +231,8 @@ def _validate_led_config(matrix, matrix_rows, matrix_cols, matrix_indexes, posit
         raise ValueError(f"LED index {max(matrix_indexes)} is OOB in g_led_config - should be < {len(flags)}")
     if not all(isinstance(n, int) for n in matrix_indexes):
         raise ValueError("matrix indexes are not all ints")
+    if not all(isinstance(n, int) for n in position_raw):
+        raise ValueError("matrix positions are not all ints")
     if (len(position_raw) % 2) != 0:
         raise ValueError("Malformed g_led_config position data")
fauxpark commented 2 weeks ago

IMO these errors are valid, and the compiler will just be rounding down anyway so I think the proper solution would be to fix the g_led_config values instead.

tzarc commented 2 weeks ago

Fixed in the actual data supplied. Decimals don't make sense.