Merge Development into main for 4.3.3 - #1600
Merged
Merged
Conversation
build_rom_selection_for_collection appended two entries per game: the original GridOrListEntry and a RomGridOrListEntry. Both classes implement get_value() returning a RomInfo, so nothing broke -- every game in a collection just rendered twice. Introduced by 746f1eb, which added the RomGridOrListEntry append as part of speeding up game list loading but never removed the old one. Keep RomGridOrListEntry, matching favorites_menu and recents_menu. Its get_description() returns folder_name, so pass the collection name there rather than the literal "Collections" -- the dropped GridOrListEntry passed description=collection, and hardcoding the literal would have replaced each row's collection name with the word "Collections". The RomGridOrListEntry also passed get_image_path_fn=lambda a, b, c: img_path, which closed over the loop variable rather than its value, so every entry resolved to the last game's box art. It was masked while the eagerly-computed GridOrListEntry rows were also in the list. Pass get_rom_select_options_builder().get_image_path directly, as favorites and recents do, which restores per-entry lazy lookup and drops the now-unused eager img_path call. Also fix favorites_menu passing folder_name="Recents", which labelled every Favorites row "Recents".
pixel2: New system image changes
pixel2: remove unused file and set default volume at first boot
Since 4.3.0 the rom listing for each folder has been cached to Saves/cache/, with the folder's modification date used to decide whether that cache is still good. That date is not trustworthy. Archive tools (7-Zip, WinRAR, Windows' built in extract) write the date stored inside the archive back onto the folder once they have finished extracting into it, so a folder can gain games while its date stays put or even moves backwards. When that happened the stale listing was kept and the new games never appeared -- permanently, since nothing ever cleared the cache. This is why it only hit some users: dragging roms in from a file manager updates the date normally, but extracting a downloaded rom pack does not. One reporter's cache showed 28 system folders sharing a single timestamp to the second, which is the fingerprint of one archive extraction stamping every folder it touched. Reading the folder is the only reliable way to tell whether it changed, but doing that before every menu draws would slow opening a system down for everyone. So the cached listing is still shown immediately and is now confirmed on a worker thread; when it turns out to be wrong the cache is corrected and a generation counter moves, which the rom menus watch so they can rebuild themselves in place. Also fixes a related case: a game added to a subfolder that was previously empty, which never showed up either because writing into a subfolder does not change the parent folder's date. - rom_list_verifier.py: worker thread, queue with per-folder dedup, generation counter - rom_utils.py: share one scan implementation between the foreground and the verifier, key the cache by directory, compare against the listing actually served so a date that shifts on its own is not mistaken for a content change - roms_menu_common.py: rebuild the list when the generation moves, keeping the selection on the same game by path - 4.3.3.sh: clear the old cache on upgrade so affected users get the right list on first open rather than one stale render - bugReport.sh: collect Saves/cache/ so this class of bug is diagnosable from a bug report
Follow-up review of the previous commit found two things worth fixing. _scan_directory was still using listdir plus an os.path.isdir call per entry, so every background verification cost one directory read plus a separate lookup for every game in the folder. scandir returns the file/directory answer as part of the read itself, so on a folder holding a few thousand games that goes from several thousand lookups to one. Extension matching no longer builds a pathlib object per file either. Symlinks are still followed, matching the os.path.isdir this replaces. Opening the game menu asks for a rom count for every active system, which queues a check for every rom folder on the card at once. Those now pause briefly between folders so the sweep doesn't sit on the card while the menu is loading box art. The generation counter is bumped before the pause, so nothing the user sees is delayed by it. Also compare listings as sets when deciding whether a folder changed. The order a directory reads back in can shift on its own, and the menu sorts the list anyway, so ordering alone should not trigger a rebuild. Verified the scandir rewrite returns identical results to the old filtering across uppercase extensions, files with no extension, trailing dots, unicode names, symlinked directories, dangling symlinks, Imgs exclusion, ignore lists, empty extlists and scanSubfolders off.
render_if_needed only redrew when the background wanted redrawing -- the next frame of an animated gif, or the next box art rotation. With a static image or a solid colour background neither of those ever comes due, so after the first render nothing redrew and the clock, date and battery sat frozen at whatever they were when the screensaver kicked in. In box art mode they only moved as often as the box art did. The widgets now schedule their own redraw. The clock renders as HH:MM and the battery moves slowly, so once a minute covers all of them, and the deadline lands on the start of the next minute so the clock ticks over when it actually changes rather than drifting. Screensavers showing only static text, or no widgets at all, schedule nothing and stay idle. Redrawing once a minute meant the static background would be decoded, and worse re-blurred, every time -- _apply_blur is a per-pixel loop in Python and would have been brutal on these devices. The decoded surface is now kept between renders and only reloaded when the image or blur setting changes. Cached as a surface rather than a texture so it survives Display.reinitialize() tearing the renderer down, matching how the box art and gif frames are already held. The next-redraw deadline is set in a finally block. A render that throws is caught and logged, and leaving the deadline in the past would have made every 5ms controller poll retry immediately, spinning the CPU and filling the log instead of failing quietly.
Sitting on a screensaver the device was still running at the menu's normal clocks with every core online, which is wasted battery when nothing is happening on screen. It now drops to the platform's powersave profile once the screensaver comes up and returns to smart the moment it is dismissed. The per-platform work was already done: set_powersave lives in cpu_control_functions.sh and every platform cfg defines its own DEVICE_MIN_CORES_ONLINE and powersave frequency range. Devices that never source cpu_control_functions.sh, MiyooMini being the one, fall through to the stub in platform/device.sh that calls set_smart instead, so they get no benefit but nothing breaks. PyUI reaches the shell functions the same way it already does for set_backlight and set_volume. Restoring happens in restore_from_blank before anything redraws, so waking does not run at idle clocks. It is also tracked with a flag so we only restore when we actually lowered it. If PyUI dies while the screensaver is up, principal.sh calls set_smart at the top of every loop, and sleep/wake calls set_performance, so a device cannot be left stuck slow. Skipped for gif and box art backgrounds. Those keep decoding and redrawing while they sit there and would visibly stutter at a single core. Controlled by a "Low power while idle" toggle in Screensaver Settings, defaulting on. Like the rest of that menu the labels fall back to inline English, since none of the screensaver keys are in the language files. Also adds the missing flag_add "setting_cpu" to set_powersave. It was the only one of the four CPU mode functions without it, so it took no lock and its closing flag_remove could clear a guard another CPU change was relying on -- more likely to matter now the screensaver calls it. display.py also picks up five lines of line ending normalisation; the file was already CRLF apart from those.
The Miyoo Mini display settings report had no way to be diagnosed from a bug report: nothing in the logs records whether the pwm backlight node exists, whether keymon is running, or whether the config file the settings write to is even present. The V4 has no wifi chip so there is no ssh route either. Dumps that state to Saves/spruce/device_state.log, which the existing include patterns already pick up. Missing nodes are reported as <missing> rather than failing, so it is harmless on platforms that have none of them.
Working out why the Miyoo Mini display settings did nothing needed facts the logs do not carry: whether the pwm backlight channel is exported, which MI libraries exist and where, what the display proc nodes contain, and whether the disp device even has a control node. The V4 has no wifi chip so there is no ssh route to any of it. Dumps all of that to Saves/spruce/device_state.log, which the existing include patterns already pick up. Also probes the disp device by opening it and asking the driver to print its own command list, which is how the real argument format was established rather than inferred. Everything is guarded, so on platforms with none of these paths it prints <missing> instead of failing.
Reported against the V4: backlight, brightness, contrast and saturation all
inoperable. Two unrelated causes, both confirmed on hardware.
The backlight had nothing to drive. /sys/class/pwm/pwmchip0 exists but was
never exported, because device_init only did so when the variant was
MIYOO_MINI_PLUS. The OG Mini and V4 fell straight past it, leaving no pwm0
node -- and none for device_exit_sleep either, which restores brightness
through that same path. The export now also runs for is_mini_og, and PyUI
writes the duty cycle itself on each change. Flip is deliberately excluded:
it does not come through this branch and its backlight already works.
The other three were only ever written into /appconfigs/system.json, which
nothing reads back, so they were stored and never applied. They now go
through the display engine.
Getting there took a while, so the dead ends are worth recording:
- The proc interface's csc command is what sprig and Allium both use. It
parses correctly, the driver accepts it, and it changes nothing here. It
was tried with all eight CscMatrix values, and with both commands down a
single handle exactly as Allium does. That block sits on the video path
and the menu is drawn into a graphics layer that never crosses it.
- colortemp on the same node does reach the panel, provably: a hard rgb
cast through it was plainly visible. Its per channel gains can carry
contrast, but its offset fields do nothing, and no arrangement of per
channel gain can desaturate, since that needs channels mixed rather than
scaled independently.
- The answer, from Koriki, is that there are two separate csc blocks. The
LCD output has its own, reachable only through the MI library, and only
once the device is configured as an lcd output and enabled. luma,
contrast, hue and saturation are all fields in that one struct.
So all four now come from a single MI_DISP_SetLcdParam call, with colour
balance left on colortemp where it belongs. The library needs libmi_sys.so
loaded RTLD_GLOBAL first or it fails on an undefined MI_SYS_Mmap, and it
lives in /config/lib rather than /customer/lib. Struct layouts and the
interface enums are taken from the SDK headers, not guessed, and verified
against the sizes the headers imply. Luma and contrast keep a floor so the
panel cannot be driven to unreadable with no way back from the menu.
Also stops _update_stock_config failing silently: /appconfigs/system.json is
empty on the V4, so json.load raised, the sed fallback ran, and sed had
nothing to match -- four no-ops per boot with nothing logged.
Known issue, to pick up next:
The screen goes fuzzy, like static, during boot and then flashes once
before settling and showing spruce cleanly. Almost certainly the
MI_DISP_SetPubAttr and MI_DISP_Enable pair reconfiguring a live display.
They currently run on every apply, so startup does it four times over as
each setting is restored. Doing it once and remembering it is the obvious
first thing to try; the calls were left as tested rather than changed
blind.
The screen showed static during boot, flashed in the menus whenever a screen
setting was applied, and left a frame of garbage sitting under the clock on the
screensaver. All three were the same bug, and all three came from the commit
that made those settings work in the first place.
Reaching the lcd output's csc needs the disp device configured and enabled, and
that was being done on the way in to every apply -- four times over at startup
as each setting was restored, and again on every change from the menu. Doing it
once fixes the flashing, but not the screensaver, which was the clue that
mattered: the reconfigure ran a single time at startup and the garbage was
still there a minute later, so it could not be what was putting it on screen.
What was, is that enabling the device does not end when the call does. Stock
never has it enabled -- the bug report dump reads DevStatus 0 with no channels
enabled -- because SDL's mmiyoo backend drives the panel through fb0 and gfx
and leaves disp alone. Enabling it switches on a layer underneath an alpha
blended osd, with nothing feeding it, so it scans out whatever happened to be
in that memory. The menus hid it because they are opaque; the screensaver, which
redraws once a minute, could not.
So the device is handed straight back. It turns out the params stay both
readable and writable with it disabled, which is now checked rather than
assumed, and checked by writing as well as reading, since writing is what
applies a setting. Confirmed on a V4: MI_DISP_Disable rc=0, and every apply
after it reads back exactly what the one before wrote. If a device ever says
otherwise it re-enables itself and says so in the log, and the settings keep
working at the cost of the layer being whatever it is.
Also worth recording, since both cost a test round:
- MI_SYS_Init is not the answer. It looked like a good candidate for the
rc=31 out of MI_DISP_GetLcdParam, being the thing every MI module wants
first and the thing Koriki calls before anything else. It returns rc=0 and
changes nothing; the read still fails. Kept as a step because it is free
and it is the one that costs nothing on screen if a variant does need it.
- Doing this in device init, before SDL takes the display, is not the answer
either. The settings applied, but the device then hung on shutdown, stuck
on the fuzz with the battery needing to be pulled. Running it after SDL is
up shuts down cleanly. Not chased further, because with the device handed
back there is no longer a reason to want it earlier.
The escalation is ordered cheapest first so it only does as much as it has to,
and each step logs its own result, which is what made the above legible at all.
Step 4 running after step 3 also matters and is not incidental: on its own
MI_DISP_GetPubAttr returns rc=31 and the struct would go out zeroed, taking the
panel timings with it, but following an enable it returns rc=0 and the timings
are read back and sent again untouched.
Applies are now also skipped when the values have not changed, so the four
settings restored at boot write once instead of three times, and the cache is
dropped after RetroArch exits since it brings the display up itself.
0cc8097 dropped aspect_ratio_index and custom_viewport_height from GBA.sh so Perfect Overlays could be offered on the Brick and BrickPro. 427 was never a general number: at 640 wide it is GBA's 3:2 to the pixel, so it only ever fitted the 640x480 devices the setting was offered on. GB.sh and GBC.sh never set either key, so GBA was the odd one out. Dropping them from the script does not get them off anyone's card, though, and two things stop it. update_config_file only rewrites the keys it is about to write, so a config that already has these two keeps them. And applyPerfectOs.sh is guarded by the perfectOverlays flag, so a re-apply does not run at all for anyone who already had the setting on. Between them the stale pair would sit there permanently, and the only way out would be toggling the setting off and back on. So it is cleaned up here instead, alongside the rom list cache, since one script covers this release. Only the exact values the old script wrote are removed, and only where both are present together, that pair being its signature. Anyone who has set their own aspect ratio or viewport height for GBA keeps it. There is deliberately no check of the perfectOverlays flag: turning the setting off already takes both keys out through remove_overlay, which still lists them. Everything in the script has to be safe to run repeatedly, which is worth spelling out because it is easy to assume otherwise from the name. The updater deletes App/spruceRestore before extracting -- it is in APP_DELETE_LIST -- and .lastUpdate lives inside it. Nothing puts it back, since it is untracked and so is not in the archive either. Every in-device update therefore starts with no .lastUpdate, the runner falls back to 2.0.0, and every upgrade script runs again. Re-running is the normal case, not an edge case. Clearing the rom cache was already safe that way; the new block is too, and it comes out byte identical to what GBA.sh now writes. Verified against fixtures rather than by eye: a config as the old script left it gets both keys taken out, one already matching current GBA.sh is untouched, one carrying a user's own custom_viewport_height is untouched, and one with only a single key of the pair is untouched. Not yet run on hardware, where the open question is busybox sed and grep on [[:space:]].
Both places that carry it: spruce/spruce, and autorun.inf, which the normal bump does not touch and which had drifted to 4.2.0 before being corrected by hand. Nothing else in the tree carries the version -- the other matches for 4.3.2 are all coincidences inside binaries. This also puts the upgrade script back in step. UpgradeScripts/4.3.3.sh was named for a 4.3.3 release while the tree said 4.3.2, which was the whole reason the two looked out of sync.
From spruceUI/RA, tag beta-a30-viewport-bias-y. md5 9b52762 -> 76ca5b1. Same ARM 32-bit ELF against the same interpreter, and the NEEDED list is unchanged at 13 entries, so nothing new has to be present on the device for it to start.
The patterns were written with [[:space:]] around the separator, which assumes
something about how the device's busybox was compiled. busybox sed and grep are
configurable at build time and reduced regex support is a real configuration, so
that was a bet rather than a fact, and losing it would have been quiet: the
pattern simply never matches, nothing is logged as wrong, and the cleanup does
nothing.
No need to bet. 4.1.2.sh already rewrites this very key, as
sed 's|^custom_viewport_height = "720"|custom_viewport_height = "768"|'
plain anchored text with single spaces. None of the upgrade scripts that have
actually run on these devices use a character class anywhere. Matching the same
way puts this on ground that is known to hold.
The spacing is safe to depend on: update_config_file appends the line with echo,
so it is written exactly like that, and RetroArch writes it the same way.
Keeping the trailing anchor is worth it on its own -- without it the viewport
pattern would also match custom_viewport_height = "4270", which is now covered
by a fixture along with the previous four.
Costs tolerance of a cfg with CRLF endings, which the old pattern would have
matched. Same silent no-op if it ever happens, and these files are written on
the device, so it is not a trade worth keeping the guesswork for.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merges
Developmentintomainfor the 4.3.3 release. 37 commits, no conflicts.Version is 4.3.3 in both
spruce/spruceandautorun.inf, matchingUpgradeScripts/4.3.3.sh.Fixes
Rom lists and collections
Screensaver
Miyoo Mini
GBA overlays (Brick / BrickPro)
Other
adbdapp hiddenra32.a30updated to the viewport-bias-y buildPixel 2
Base support work from @Hairo — system image changes, CPU/GPU values, LED settings, emulator and first-boot fixes, plus a usbstorage reboot fix that affects other devices too.
Testing notes
A few of these have not been exercised end to end on hardware yet and are worth a look before the release goes out: