fix(display): refuse settings the rgbmatrix library aborts on, on every board

The library answers several settings with a NULL matrix or abort() rather
than an error, so the display service crash-looped (Restart=on-failure)
instead of reaching fallback mode: rows above 64, chain_length above 255
(uint8_t binding setter, documented as "no upper limit"), a misspelled
hardware_mapping, and parallel 2-3 on a single-output mapping, reachable
from the Display form on the default adafruit-hat(-pwm) mapping. #586 only
guarded the Pi 5 subset.

- src/matrix_support.py holds the rules for every board (Options::Validate
  ranges, binding integer types, mapping names and outputs from
  lib/hardware-mapping.c) plus the Pi 5 ones, and is the one source of the
  API's numeric ranges.
- DisplayManager checks them before building options and raises
  MatrixSettingsRefused, so a hand-edited config falls back with a logged,
  reported reason. Emulator mode only warns.
- The config API refuses them with a 400 naming the setting; combinations
  are checked against stored values but reported only when the request
  sets a field involved.
- The hardware status file gains "cause" (settings/library/forced). The
  fallback log and Display banner give the Pi 5 rebuild hint only for a
  library failure instead of rebuild + gpio_slowdown advice for every
  failure; one Pi 5 slowdown recommendation (1-3, start at 1).
- The Display form offers classic/classic-pi1 and orientation 90/270 and
  renders any other stored mapping selected with a warning, so an
  unrelated save no longer rewrites them; the API accepts 90/270.

Audit findings F03, F16, F19, F21.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-09-17 08:59:47 -04:00
co-authored by Claude Opus 5
parent c693278d33
commit 94ec74caa5
7 changed files with 680 additions and 103 deletions
+118 -10
View File
@@ -1,9 +1,10 @@
"""Display hardware settings accept what the rgbmatrix library accepts.
Held to the ranges in the pinned library (RGBMatrix::Options::Validate in
lib/options-initialize.cc, the gpio_slowdown check in lib/led-matrix.cc), with
one deliberate exception: rows has no upper bound here, although the library
currently rejects more than 64 per panel. Two ways this used to go wrong:
Held to what the pinned library and its Python binding accept
(src/matrix_support.py: RGBMatrix::Options::Validate in
lib/options-initialize.cc, the gpio_slowdown check in lib/led-matrix.cc, the
mapping table in lib/hardware-mapping.c and the binding's uint8_t setters).
Ways this used to go wrong:
- The Display form capped cols at 128, chain_length at 24 and
pwm_lsb_nanoseconds at 500, and its submit handler (fixInvalidNumberInputs)
@@ -11,6 +12,10 @@ currently rejects more than 64 per panel. Two ways this used to go wrong:
a long chain silently saved as the wrong size.
- The API checked none of these, so a value the library rejects (odd rows,
parallel 4, pwm_dither_bits 3) saved, and the matrix then refused to start.
- After that, rows above 64, chain_length above 255, a misspelled hardware
mapping and parallel 2-3 on a single-output HAT mapping still saved. The
library answers those with no matrix or abort(), not an error, so the
display service crash-looped instead of falling back.
Row address type 5 is the SM5368 / B707 row shift register the Waveshare 96x48
V2 needs (Waveshare's own "96X48_1_24_SM5368" panel type in their library fork
@@ -102,10 +107,9 @@ def test_waveshare_96x48_v2_settings_all_save(api_v3_client, saved, as_strings):
@pytest.mark.parametrize('field,value', [
('rows', 8), ('rows', 64), ('rows', 96), ('rows', 128),
('rows', 8), ('rows', 64),
('cols', 16), ('cols', 192), ('cols', 512),
('chain_length', 1), ('chain_length', 32),
('parallel', 3),
('chain_length', 1), ('chain_length', 32), ('chain_length', 255),
('row_address_type', 0), ('row_address_type', 5), ('row_address_type', 5.0),
('multiplexing', 0), ('multiplexing', 22),
('gpio_slowdown', 0), ('gpio_slowdown', 10),
@@ -123,9 +127,9 @@ def test_values_in_range_are_saved(api_v3_client, saved, field, value):
@pytest.mark.parametrize('field,value', [
('rows', 6), ('rows', 47), ('rows', 97), ('rows', '48.5'),
('rows', 6), ('rows', 47), ('rows', 66), ('rows', 96), ('rows', 128), ('rows', '48.5'),
('cols', 15), ('cols', 96.5), ('cols', True), ('cols', 'wide'),
('chain_length', 0),
('chain_length', 0), ('chain_length', 256), ('chain_length', 300),
('parallel', 0), ('parallel', 4),
('row_address_type', -1), ('row_address_type', 6),
('row_address_type', True), ('row_address_type', 5.5),
@@ -146,6 +150,69 @@ def test_values_out_of_range_are_refused(api_v3_client, saved, field, value):
assert 'config' not in saved
@pytest.mark.parametrize('body', [
{'hardware_mapping': 'regular', 'parallel': 3},
{'hardware_mapping': 'classic', 'parallel': 2},
{'hardware_mapping': 'Regular', 'parallel': 3},
{'hardware_mapping': 'classic-pi1'},
{'hardware_mapping': 'adafruit-hat', 'parallel': 1},
])
def test_mappings_the_library_has_are_saved(api_v3_client, saved, body):
response = _post(api_v3_client, body)
assert response.status_code == 200, response.get_data(as_text=True)[:200]
for field, value in body.items():
assert saved['config']['display']['hardware'][field] == value
@pytest.mark.parametrize('body,named', [
# Framebuffer() abort()s: the HAT mappings define one output.
({'hardware_mapping': 'adafruit-hat-pwm', 'parallel': 2}, 'parallel 2'),
({'hardware_mapping': 'adafruit-hat', 'parallel': 3}, 'parallel 3'),
({'hardware_mapping': 'regular-pi1', 'parallel': 2}, 'parallel 2'),
# InitHardwareMapping() abort()s on a name it doesn't have; compute-module
# isn't compiled into the default build.
({'hardware_mapping': 'adafruit-hat-pwn'}, 'adafruit-hat-pwn'),
({'hardware_mapping': 'compute-module'}, 'compute-module'),
({'hardware_mapping': 5}, 'hardware mapping'),
])
def test_combinations_the_library_aborts_on_are_refused(api_v3_client, saved, body, named):
response = _post(api_v3_client, body)
assert response.status_code == 400
assert named in response.get_json()['message']
assert 'config' not in saved
def test_parallel_is_checked_against_the_stored_mapping(api_v3_client, api_v3_module, saved):
api_v3_module.api_v3.config_manager.load_config.return_value = {
'display': {'hardware': {'hardware_mapping': 'adafruit-hat'}}}
response = _post(api_v3_client, {'parallel': 2})
assert response.status_code == 400
assert 'adafruit-hat' in response.get_json()['message']
assert 'config' not in saved
def test_stored_refusal_does_not_block_unrelated_saves(api_v3_client, api_v3_module, saved):
api_v3_module.api_v3.config_manager.load_config.return_value = {
'display': {'hardware': {'hardware_mapping': 'adafruit-hat', 'parallel': 2}}}
response = _post(api_v3_client, {'brightness': 70})
assert response.status_code == 200, response.get_data(as_text=True)[:200]
def test_a_request_value_is_checked_not_the_stored_one(api_v3_client, api_v3_module, saved):
"""Fixing a stored bad value in the same save as a mapping change must work."""
api_v3_module.api_v3.config_manager.load_config.return_value = {
'display': {'hardware': {'rows': 128, 'parallel': 2}}}
response = _post(api_v3_client, {'rows': 64, 'hardware_mapping': 'regular'})
assert response.status_code == 200, response.get_data(as_text=True)[:200]
@pytest.mark.parametrize('orientation', ['normal', '90', '180', '270'])
def test_every_orientation_display_manager_applies_is_saved(api_v3_client, saved, orientation):
response = _post(api_v3_client, {'orientation': orientation})
assert response.status_code == 200, response.get_data(as_text=True)[:200]
assert saved['config']['display']['hardware']['orientation'] == orientation
@pytest.fixture
def display_page(monkeypatch):
"""Render the Display settings partial for a given config."""
@@ -231,6 +298,38 @@ def test_waveshare_96x48_v2_config_renders_back_unchanged(display_page):
assert _attr(_input_tag(body, input_id), 'value') == str(WAVESHARE_96X48_V2[input_id]), input_id
@pytest.mark.parametrize('hardware,select_id,expected', [
({'hardware_mapping': 'classic'}, 'hardware_mapping', 'classic'),
({'hardware_mapping': 'classic-pi1'}, 'hardware_mapping', 'classic-pi1'),
({'hardware_mapping': 'adafruit-hat'}, 'hardware_mapping', 'adafruit-hat'),
({'hardware_mapping': 'Regular'}, 'hardware_mapping', 'regular'),
({'hardware_mapping': ''}, 'hardware_mapping', 'regular'),
({'orientation': '90'}, 'orientation', '90'),
({'orientation': '270'}, 'orientation', '270'),
])
def test_stored_mapping_and_orientation_render_back_unchanged(display_page, hardware, select_id, expected):
"""With nothing selected the browser posts the first option, so one unrelated
Display save turned classic into adafruit-hat-pwm and 90 degrees into normal."""
body = display_page(_config_with(hardware=hardware))
assert _selected_option(body, select_id) == [expected]
assert "saved hardware mapping" not in body
def test_missing_mapping_renders_display_managers_default(display_page):
config = _config_with()
del config['display']['hardware']['hardware_mapping']
assert _selected_option(display_page(config), 'hardware_mapping') == ['adafruit-hat-pwm']
@pytest.mark.parametrize('stored', ['compute-module', 'adafruit-hat-pwn'])
def test_unusable_stored_mapping_renders_selected_with_a_warning(display_page, stored):
"""Kept selected rather than silently swapped for the first option; the API
refuses it on save, and the warning says why."""
body = display_page(_config_with(hardware={'hardware_mapping': stored}))
assert _selected_option(body, 'hardware_mapping') == [stored]
assert f'Your saved hardware mapping ("{stored}")' in body
@pytest.mark.parametrize('field,section', [
('gpio_slowdown', 'runtime'), ('pwm_dither_bits', 'hardware'),
('limit_refresh_rate_hz', 'hardware'),
@@ -248,7 +347,7 @@ def test_a_stored_zero_renders_as_zero(display_page, field, section):
@pytest.mark.parametrize('body', [
{'row_address_type': 5}, {'row_address_type': '1'},
{'hardware_mapping': 'compute-module'},
{'hardware_mapping': 'classic-pi1'},
])
def test_pi5_refuses_what_its_library_cannot_drive(api_v3_client, saved, board, body):
board(PI5_MODEL)
@@ -296,6 +395,15 @@ def test_pi5_form_offers_only_supported_row_address_types(display_page, board):
assert "can't be used on this Raspberry Pi 5" not in body
def test_pi5_form_offers_only_mappings_it_can_drive(display_page, board):
board(PI5_MODEL)
body = display_page(_config_with())
assert 'classic-pi1' not in _option_values(body, 'hardware_mapping')
body = display_page(_config_with(hardware={'hardware_mapping': 'classic-pi1'}))
assert _selected_option(body, 'hardware_mapping') == ['classic-pi1']
assert 'can use on a Raspberry Pi 5' in body
def test_other_boards_offer_every_row_address_type(display_page):
body = display_page(_config_with())
assert _option_values(body, 'row_address_type') == ['0', '1', '2', '3', '4', '5']
+99 -5
View File
@@ -265,6 +265,14 @@ class TestDisplayManagerOrientation:
options = mock_rgb_matrix['options_class'].return_value
assert options.pixel_mapper_config == ''
@pytest.mark.parametrize('orientation', ['90', '270'])
def test_sideways_orientation_appends_rotate_mapper(self, mock_rgb_matrix, orientation):
DisplayManager._instance = None
with patch.dict('os.environ', {'EMULATOR': 'false'}):
DisplayManager(self._config(orientation=orientation), suppress_test_pattern=True)
options = mock_rgb_matrix['options_class'].return_value
assert options.pixel_mapper_config == f'Rotate:{orientation}'
def test_orientation_180_appends_rotate_mapper(self, mock_rgb_matrix):
DisplayManager._instance = None
with patch.dict('os.environ', {'EMULATOR': 'false'}):
@@ -281,10 +289,11 @@ class TestDisplayManagerOrientation:
assert options.pixel_mapper_config == 'U-mapper;Rotate:180'
class TestDisplayManagerPi5Guard:
"""On a Pi 5 the library returns no matrix for settings its RP1 path can't
drive, and the binding doesn't check, so the process would crash on its next
call. DisplayManager has to refuse before creating the matrix and fall back."""
class TestDisplayManagerLibraryGuard:
"""For many settings it can't use the library returns no matrix (which the
binding doesn't check, so the process crashes on its next call) or calls
abort() -- on every board, with more on a Pi 5. DisplayManager has to refuse
before creating the matrix and fall back, reporting why."""
def _config(self, **hardware_overrides):
config = {
@@ -327,10 +336,95 @@ class TestDisplayManagerPi5Guard:
mock_rgb_matrix['matrix_class'].assert_called_once()
assert dm.matrix is not None
def test_other_boards_are_left_to_the_library(self, mock_rgb_matrix, board):
def test_pi5_only_limits_do_not_apply_to_other_boards(self, mock_rgb_matrix, board):
board('Raspberry Pi 4 Model B Rev 1.5')
DisplayManager._instance = None
with patch.dict('os.environ', {'EMULATOR': 'false'}):
dm = DisplayManager(self._config(row_address_type=5), suppress_test_pattern=True)
mock_rgb_matrix['matrix_class'].assert_called_once()
assert dm.matrix is not None
@pytest.fixture
def hw_status(self, tmp_path, monkeypatch):
"""What DisplayManager writes to /tmp/led_matrix_hw_status.json."""
import json as _json
import tempfile as _tempfile
from src import display_manager as dm_module
written = {}
real_replace = os.replace
real_mkstemp = _tempfile.mkstemp
def fake_mkstemp(dir=None, prefix=None):
return real_mkstemp(dir=str(tmp_path), prefix=prefix)
def fake_replace(src, dst):
if str(dst).endswith('led_matrix_hw_status.json'):
with open(src) as f:
written.update(_json.load(f))
os.remove(src)
else:
real_replace(src, dst)
monkeypatch.setattr(dm_module.tempfile, 'mkstemp', fake_mkstemp)
monkeypatch.setattr(dm_module.os, 'replace', fake_replace)
monkeypatch.setattr(dm_module.os.path, 'islink', lambda _p: False)
return written
@pytest.mark.parametrize('overrides,named', [
({'hardware_mapping': 'adafruit-hat-pwm', 'parallel': 2}, 'parallel 2'),
({'hardware_mapping': 'adafruit-hat', 'parallel': 3}, 'parallel 3'),
({'hardware_mapping': 'adafruit-hat-pwn'}, 'adafruit-hat-pwn'),
({'rows': 128}, 'rows 128'),
({'chain_length': 300}, 'chain_length 300'),
])
def test_hand_edited_setting_the_library_refuses_falls_back_on_any_board(
self, mock_rgb_matrix, board, hw_status, overrides, named):
board('Raspberry Pi 4 Model B Rev 1.5')
DisplayManager._instance = None
with patch.dict('os.environ', {'EMULATOR': 'false'}):
dm = DisplayManager(self._config(**overrides), suppress_test_pattern=True)
mock_rgb_matrix['matrix_class'].assert_not_called()
assert dm.matrix is None
assert hw_status['ok'] is False
assert hw_status['cause'] == 'settings'
assert named in hw_status['error']
def test_library_failure_is_reported_as_the_library(self, mock_rgb_matrix, board, hw_status):
board('Raspberry Pi 4 Model B Rev 1.5')
mock_rgb_matrix['matrix_class'].side_effect = RuntimeError('boom')
DisplayManager._instance = None
with patch.dict('os.environ', {'EMULATOR': 'false'}):
dm = DisplayManager(self._config(), suppress_test_pattern=True)
assert dm.matrix is None
assert hw_status == {'ok': False, 'error': 'boom', 'cause': 'library'}
def test_success_reports_no_cause(self, mock_rgb_matrix, board, hw_status):
board('Raspberry Pi 4 Model B Rev 1.5')
DisplayManager._instance = None
with patch.dict('os.environ', {'EMULATOR': 'false'}):
DisplayManager(self._config(), suppress_test_pattern=True)
assert hw_status == {'ok': True, 'error': None, 'cause': None}
def test_emulator_warns_but_still_starts(self, mock_rgb_matrix, board, caplog):
board('Raspberry Pi 4 Model B Rev 1.5')
DisplayManager._instance = None
with patch.dict('os.environ', {'EMULATOR': 'true'}):
dm = DisplayManager(self._config(rows=128), suppress_test_pattern=True)
mock_rgb_matrix['matrix_class'].assert_called_once()
assert dm.matrix is not None
assert 'rows 128' in caplog.text
def test_refused_settings_advice_does_not_send_users_to_rebuild(self, board):
"""The Pi 5 rebuild / GPIO slowdown hint used to follow every failure,
including settings LEDMatrix itself refused."""
from src.matrix_support import MatrixSettingsRefused
board('Raspberry Pi 5 Model B Rev 1.0')
advice = DisplayManager._fallback_advice(
'settings', MatrixSettingsRefused('row address type 5'))
assert 'Display tab' in advice
assert 'first_time_install' not in advice and 'slowdown' not in advice
library = DisplayManager._fallback_advice('library', RuntimeError('mmap failed'))
assert 'RPI_RGB_FORCE_REBUILD=1' in library
board('Raspberry Pi 4 Model B Rev 1.5')
assert 'RPI_RGB_FORCE_REBUILD' not in DisplayManager._fallback_advice(
'library', RuntimeError('boom'))
+125
View File
@@ -0,0 +1,125 @@
"""The library-refusal rules in src/matrix_support.py mirror the pinned library.
rpi-rgb-led-matrix-master at 1ee4f76: RGBMatrix::Options::Validate in
lib/options-initialize.cc, the runtime checks in RGBMatrix::CreateFromOptions
(lib/led-matrix.cc), the mapping table in lib/hardware-mapping.c with the
abort()s in lib/framebuffer.cc, and the setter types in
bindings/python/rgbmatrix/core.pyx. When the submodule is bumped and those
change, these are the cases to revisit.
"""
import json
import sys
from pathlib import Path
import pytest
sys.path.insert(0, str(Path(__file__).parent.parent))
from src import matrix_support as ms # noqa: E402
REPO_ROOT = Path(__file__).resolve().parents[1]
def _messages(hardware, runtime=None, pi5=False):
return [r.message for r in ms.library_refusals(hardware, runtime, pi5=pi5)]
def test_the_config_template_starts():
template = json.loads((REPO_ROOT / 'config' / 'config.template.json').read_text(encoding='utf-8'))
display = template['display']
assert ms.library_refusals(display['hardware'], display['runtime']) == []
assert ms.library_refusals(display['hardware'], display['runtime'], pi5=True) == []
def test_display_manager_defaults_start():
assert ms.library_refusals({}) == []
@pytest.mark.parametrize('hardware', [
{'rows': 8}, {'rows': 64}, {'cols': 16}, {'cols': 1024},
{'chain_length': 1}, {'chain_length': 255},
{'hardware_mapping': 'regular', 'parallel': 3},
{'hardware_mapping': 'classic', 'parallel': 3},
{'hardware_mapping': 'REGULAR', 'parallel': 2},
{'hardware_mapping': '', 'parallel': 3}, # empty reads as "regular"
{'hardware_mapping': 'classic-pi1'},
{'hardware_mapping': 'regular-pi1', 'parallel': 1},
{'pwm_bits': 11}, {'pwm_dither_bits': 2}, {'pwm_lsb_nanoseconds': 3000},
{'row_address_type': 5}, {'multiplexing': 22}, {'scan_mode': 1},
{'brightness': 1}, {'limit_refresh_rate_hz': 0},
{'led_rgb_sequence': 'bgr'}, {'led_rgb_sequence': 'GBR'},
# Not a number: left to the binding, which raises a TypeError.
{'rows': 'thirty-two'},
])
def test_what_the_library_starts_with(hardware):
assert ms.library_refusals(hardware) == []
@pytest.mark.parametrize('hardware,runtime,named', [
({'rows': 66}, None, 'rows 66'),
({'rows': 128}, None, 'rows 128'),
({'rows': 31}, None, 'rows 31'),
({'cols': 8}, None, 'cols 8'),
({'chain_length': 0}, None, 'chain_length 0'),
({'chain_length': 256}, None, 'chain_length 256'), # uint8_t setter
({'hardware_mapping': 'regular', 'parallel': 4}, None, 'parallel 4'),
({'hardware_mapping': 'adafruit-hat-pwm', 'parallel': 2}, None, 'which has 1 output'),
({'hardware_mapping': 'adafruit-hat', 'parallel': 3}, None, 'which has 1 output'),
({'hardware_mapping': 'regular-pi1', 'parallel': 2}, None, 'which has 1 output'),
({'hardware_mapping': 'classic-pi1', 'parallel': 2}, None, 'which has 1 output'),
({'parallel': 2}, None, 'adafruit-hat-pwm'), # DisplayManager's default mapping
({'hardware_mapping': 'adafruit-hat-pwn'}, None, '"adafruit-hat-pwn"'),
({'hardware_mapping': 'compute-module'}, None, '"compute-module"'),
({'hardware_mapping': None}, None, 'hardware mapping None'),
({'brightness': 0}, None, 'brightness 0'),
({'pwm_bits': 12}, None, 'pwm_bits 12'),
({'pwm_dither_bits': 3}, None, 'pwm_dither_bits 3'),
({'pwm_lsb_nanoseconds': 49}, None, 'pwm_lsb_nanoseconds 49'),
({'row_address_type': 6}, None, 'row_address_type 6'),
({'multiplexing': 23}, None, 'multiplexing 23'),
({'scan_mode': 2}, None, 'scan_mode 2'),
({'led_rgb_sequence': 'RGBW'}, None, 'LED RGB sequence'),
({'led_rgb_sequence': 'RRB'}, None, 'LED RGB sequence'),
({}, {'gpio_slowdown': 11}, 'gpio_slowdown 11'),
({}, {'gpio_slowdown': -1}, 'gpio_slowdown -1'),
({}, {'rp1_rio': 2}, 'rp1_rio 2'),
])
def test_what_it_refuses(hardware, runtime, named):
messages = _messages(hardware, runtime)
assert any(named in m for m in messages), messages
def test_pi5_limits_apply_only_on_a_pi5():
assert ms.library_refusals({'row_address_type': 5}) == []
refusals = ms.library_refusals({'row_address_type': 5}, pi5=True)
assert len(refusals) == 1 and refusals[0].pi5
assert set(refusals[0].fields) == {'row_address_type', 'parallel', 'hardware_mapping'}
def test_refusal_names_the_fields_involved():
(refusal,) = ms.library_refusals({'hardware_mapping': 'adafruit-hat', 'parallel': 2})
assert set(refusal.fields) == {'parallel', 'hardware_mapping'}
(refusal,) = ms.library_refusals({'rows': 128})
assert refusal.fields == ('rows',)
def test_message_names_every_problem():
message = ms.refusal_message(ms.library_refusals(
{'rows': 128, 'row_address_type': 5}, {'gpio_slowdown': 11}, pi5=True))
assert message.startswith("The installed rgbmatrix library can't start")
assert 'rows 128' in message and 'gpio_slowdown 11' in message
assert 'Raspberry Pi 5' in message and 'row address type 5' in message
assert ms.refusal_message([]) is None
def test_non_mapping_sections_are_treated_as_empty():
assert ms.library_refusals('oops', ['a']) == []
def test_display_manager_defaults_match_display_manager():
"""The guard fills missing keys the way DisplayManager._setup_matrix does."""
source = (REPO_ROOT / 'src' / 'display_manager.py').read_text(encoding='utf-8')
for field, value in ms.DISPLAY_MANAGER_DEFAULTS.items():
if field in ('rows', 'cols', 'chain_length', 'parallel'):
continue # read through display_geometry's DEFAULT_* constants
assert f"get('{field}', {value!r})" in source, field