osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/simtrace2/+/26463 )
Change subject: firmware: add crc stub to all dfu apps to ensure reliable loading ...................................................................... Patch Set 3: Code-Review+1 (1 comment) Code looks good to me. Regarding linter errors from https://jenkins.osmocom.org/jenkins/job/gerrit-simtrace2-lint/49/console: 1. firmware/libcommon/source/crcstub.c:31: WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst Doesn't make sense here IMHO, submitted a patch to ignore it: https://gerrit.osmocom.org/c/osmo-ci/+/26464 2. firmware/misc/crctool.c:77: WARNING:BRACES: braces {} are not necessary for single statement blocks Doesn't seem useful either: https://gerrit.osmocom.org/c/osmo-ci/+/26465 3. firmware/libcommon/source/crcstub.c:16: ERROR:FSF_MAILING_ADDRESS: Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The FSF has changed addresses in the past, and may do so again. Linux already includes a copy of the GPL. Personally I like if there is not so much boilerplate at the start of every file, and having the mailing address from the FSF in there in 2021 is probably not useful. So I'd argue it makes sense to remove the address. https://gerrit.osmocom.org/c/simtrace2/+/26463/2/firmware/Makefile File firmware/Makefile: https://gerrit.osmocom.org/c/simtrace2/+/26463/2/firmware/Makefile@288 PS2, Line 288: m > a subsequent repeated 'make' command will no longer try executing crctool, as > the .bin already exists? No, because "dfu" is a PHONY target. In case one would run the "dfu" target twice (although Eric says above that this usually is not done/possible?), it will attempt to run the crctool again and fail because the magic does not match anymore. I agree that it would be cleaner to have it use a separate file, but it looks like this is not a problem. -- To view, visit https://gerrit.osmocom.org/c/simtrace2/+/26463 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: simtrace2 Gerrit-Branch: master Gerrit-Change-Id: Id6df0486c8b779889d21800dc2441b3aa9af8a5f Gerrit-Change-Number: 26463 Gerrit-PatchSet: 3 Gerrit-Owner: Hoernchen <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-Reviewer: tsaitgaist <[email protected]> Gerrit-Comment-Date: Wed, 08 Dec 2021 11:16:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Hoernchen <[email protected]> Comment-In-Reply-To: laforge <[email protected]> Gerrit-MessageType: comment
