For such controllers, we terminate all multi-block transfers using a subsequent CMD12 rather than by prefixing them with CMD23. Fixes misbehaviour observed with the SDHOST controller which is now used for the SD slot on WiFi-capable Raspberry Pi models prior to the Pi 4, and for any SD card HATs on generation-4 Raspberry Pis.
Requires symbols exported from RiscOS/Sources/HWSupport/SD/SDIODriver!8 to build.
ROOL (e6d25ce0) at 14 Mar 18:21
Support for controllers that misbehave when they see CMD23
For such controllers, we terminate all multi-block transfers using a subsequent CMD12 rather than by prefixing them with CMD23. Fixes misbehaviour observed with the SDHOST controller which is now used for the SD slot on WiFi-capable Raspberry Pi models prior to the Pi 4, and for any SD card HATs on generation-4 Raspberry Pis.
Requires symbols exported from RiscOS/Sources/HWSupport/SD/SDIODriver!8 to build.
Some controllers (such as the eMMC controller in the Pinebook) require transfers to be limited to a smaller number of blocks per operation than is permitted by a generic SDHCI controller.
Requires RiscOS/Sources/HWSupport/SD/SDIODriver!6 in order to build.
Requires Support/CI_Source!24 in order to pass CI.
ROOL (3483d3d4) at 18 Jul 13:24
Support optional factory-fit non-removable memory devices
... and 2 more commits
The comment is no longer present, so this issue has been resolved.
This issue has now been resolved.
I've distilled the essence of the variadic macro sorcery into bavison/PreproLib> because it doesn't really feel like it belongs here - it's generally reusable elsewhere. I've also realised I can use the same trick for lots of other common macros that could usefully be variadic.
In the meantime, I've backed it out from here and replaced it with a simpler construct that makes it clear under which build switch combination the variable ends up being unused.
Some controllers (such as the eMMC controller in the Pinebook) require transfers to be limited to a smaller number of blocks per operation than is permitted by a generic SDHCI controller.
Requires RiscOS/Sources/HWSupport/SD/SDIODriver!6 in order to build.
Requires Support/CI_Source!24 in order to pass CI.
This level of preprocessor magic needs a comment to explain what's going on at the very least.
Latest update
unreadVariable
suppression which itself now causes a cppcheck
warning now that dprintf
evaluates all its arguments (overlooked because Support/CI_Source!12 is still pending - without that, cppcheck
jobs will always fail for all modules)That's actually the same issue Jeffrey raised above. The reason is that cppcheck
simultaneously checks all possible combinations of preprocessor switches (unless you override by passing extra arguments on its command line), and it complains if any combination of switches leaves the value unused. It could be argued that's a bug in cppcheck
. It could also be argued that it's partly because RISC OS service call handlers aren't allowed to return an error pointer (otherwise I'd just have returned it from the function, and cppcheck
would have been happy). You could also say we ought to have some way of logging errors that occur within service call handlers. All of those are out of scope for this MR though.
I've tried an alternative tack, based on the argument that the problem is that in some (i.e. non-debug) build variants, dprintf
fails to evaluate any of its arguments. (A well-behaved macro in C should really evaluate each of its arguments exactly once.) This is made rather trickier by the fact that dprintf
is variadic, but I've found a way to do so - at least as long as there aren't an enormous number of arguments! This is enough to get cppcheck
to shut up even in the absence of the inline suppression comment, so I've been able to remove that as a result.
Aside: the fact that dprintf
now evaluates each argument exactly once in all variants means that it's now valid to pass it arguments with side-effects, such as:
dprintf("Skipping value %u\n", *ptr++);
Done.
e is read though, right on the next line
OSHW_DeviceEnumerateChrono
I've had a shot at an extension to the CI jobs to check that debug
builds work: Support/CI_Source!19
Turns out dprintf
did nothing for either debug or release builds. I've changed it so debug builds define DEBUG_ENABLED
, which would have made it easier to spot this.
Weirdly, although cppcheck
does simultaneously test all possible combinations of build switches, it was complaining about the value assigned to e
not being used in only some combinations. Personally I'd consider that to be a cppcheck
bug. I'll have to find out how to report it.
It's not a bad idea to have a CI job to test that make debug
completes successfully. I've considered it myself before - I think I've shied away from it because not every Makefile defines such a target (any of the libraries for a start). Support/CI_Source!15 (merged) would go some way to dealing with that when it's merged though.
Removing e
and its assignments breaks many of the dprintfs.
It'd be nice if (a) the CI pipeline didn't complain about this (can we tell it to not complain about unused variables which are used by dprintf?), and (b) the CI pipeline could check that debug builds build correctly (at least for commonly used debug options); I think I've come across a few components where the debug code has become broken over time and no longer builds/works.
Some controllers (such as the eMMC controller in the Pinebook) require transfers to be limited to a smaller number of blocks per operation than is permitted by a generic SDHCI controller.
Requires RiscOS/Sources/HWSupport/SD/SDIODriver!6 in order to build.
Requires Support/CI_Source!24 in order to pass CI.