[riot-notifications] [RIOT-OS/RIOT] Improve shell_commands (#8564)

Juan I Carrano notifications at github.com
Tue Mar 5 12:10:21 CET 2019


jcarrano commented on this pull request.

I think this makes the shell module much more cleaner and it _is_ an improvement.

In my view, with full XFA support, the ideal solution will be for the shell commands to be defined by the module to which they relate (eg. the "can" command would be defined in the "can" module, not in "shell"). Still, this is a good stepping stone in that direction.

The hardest blocker for this at the moment is that the array is placed in RAM, thus wasting both flash _and_ RAM.

> @@ -115,7 +115,7 @@ SECTIONS
         _estack = .;
     } > ram
 
-    .relocate : AT (_etext)
+    .data : AT (_etext)

What does this do? Can this renaming of the section affect something else?

> @@ -86,3 +86,12 @@ endif
 ifneq (native,$(BOARD))
   INCLUDES += -I$(RIOTBASE)/sys/libc/include
 endif
+
+ifneq (,$(filter shell_commands,$(USEMODULE)))
+ifneq (,$(filter cortexm_common,$(USEMODULE)))
+  LINKFLAGS += $(RIOTBASE)/sys/shell/commands/shell_commands_cortexm.ld
+else
+  LINKFLAGS += $(RIOTBASE)/sys/shell/commands/shell_commands.ld
+endif
+  UNDEF += -Wl,--whole-archive $(BINDIR)/shell_commands.a  -Wl,--no-whole-archive

Mmm, interesting. I was wondering how you avoided the linker discarding the symbols, which is the reason why #9105 is still unmerged: it needs either #8711 (ld -r) or "--whole-archive" (which I think is the preferred option in conjuction with #10195 )

I see that you avoided much of the problems that have been holding #8711 by limiting the "--whole-archive" to just the shell module.

@cladmi , @gebart Wouldn't this be a good approach "unlock" #8711, or its equivalent with "--whole-archive"? As far as I understand, most of the problems come from a minority of files which define things like interrupt handlers. The "--whole-archive" could be enabled only for select modules (or, if most things work, enabled by default and disabled for select modules.)

> @@ -0,0 +1,9 @@
+SECTIONS
+{
+  .data :
+  {
+    _shell_command_list = .; /* begining of the command list */
+    KEEP(*(._shell_command_list))
+    KEEP(*(._shell_command_list_end))
+  } > ram

Why is it necessary to put it in RAM in cortex-m?

-- 
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/8564#pullrequestreview-210616105
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190305/783d7382/attachment-0001.html>


More information about the notifications mailing list