Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Layout & RGB Matrix coordinate data type #23940

Closed
wants to merge 1 commit into from

Conversation

waffle87
Copy link
Member

Description

Title

Types of Changes

  • Core
  • Bugfix

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the dd Data Driven Changes label Jun 16, 2024
@zvecr
Copy link
Member

zvecr commented Jun 17, 2024

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
Copy link
Member

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
Copy link
Member

tzarc commented Jun 17, 2024

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

@tzarc tzarc closed this Jun 17, 2024
@waffle87 waffle87 deleted the fix/coordinate_data branch June 19, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dd Data Driven Changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants