Change in simtrace2[master]: firmware: add crc stub to all dfu apps to ensure reliable loading

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

osmith gerrit-no-reply at lists.osmocom.org
Wed Dec 8 11:16:11 UTC 2021


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 <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: tsaitgaist <kredon at sysmocom.de>
Gerrit-Comment-Date: Wed, 08 Dec 2021 11:16:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Hoernchen <ewild at sysmocom.de>
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211208/633e97e3/attachment.htm>


More information about the gerrit-log mailing list