[riot-notifications] [RIOT-OS/RIOT] riotboot: add riotboot_flashwrite module (#11181)

Gaƫtan Harter notifications at github.com
Tue Mar 19 14:28:21 CET 2019


I disagree, the fix from https://github.com/RIOT-OS/RIOT/pull/11203 do not fix the issues.

This pull request changed the API without mentioning in the title or giving an adequate testing description.

The third commit https://github.com/RIOT-OS/RIOT/pull/11181/commits/a15f07b04bb3153c69c7dfcb3aa9b5cb23724c07 is added without justification and changes the definition scope of the makefiles variables and made some non namespaced macros globally defined.
This may be correct, but not without a specific discussion and a justification other than "move slot variables". It should have be in its own pull request. Also, if the variables are now parsed by Makefile.include why keep the export ?

If it was required for https://github.com/RIOT-OS/RIOT/pull/11181/commits/ae35860510915461544384ccde57d9455095f18d why is it defined after the commit introducing it ?

And the fix proposed in #11203  now makes that having `FEATURES_REQUIRED+=riotboot` define global non namespaced macros for no reasons.


This should really have been split to a separate pull request with dedicated testing procedure and justification as this pull request did not gave any and did not mention it either.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11181#issuecomment-474368399
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190319/f4c2d120/attachment.html>


More information about the notifications mailing list