cvra / robot-software

CVRA monorepo - All software running on our bots lives here
MIT License
43 stars 21 forks source link

Register shell commands using linker #231

Closed msplr closed 5 years ago

msplr commented 5 years ago

This PR changes the way shell commands are registered in the commands list. Instead of manually keeping a list of available commands, one can simply define a shell command using the macro SHELL_COMMAND(name, chp, argc, argv). In addition to defining the function, the macro also defines a list entry struct placed in .shell_commands section located in flash.

This also allows to define a shell command anywhere in the code which helps decoupling the modules.

SyrianSpock commented 5 years ago

Hardware panics when I launch the USB shell from my laptop :crying_cat_face:

(gdb) bt
#0  panic_hook (reason=reason@entry=0x805151c "SV#6") at ./src/main.c:104
#1  0x08001aa4 in chSysHalt (reason=reason@entry=0x805151c "SV#6") at ../lib/ChibiOS/os/rt/src/chsys.c:209
#2  0x08001d7c in _dbg_check_lock_from_isr () at ../lib/ChibiOS/os/rt/src/chdebug.c:179
#3  0x08001b96 in chSysLockFromISR () at ../lib/ChibiOS/os/rt/include/chsys.h:397
#4  chSysGetStatusAndLockX () at ../lib/ChibiOS/os/rt/src/chsys.c:377
#5  0x0802c376 in trace_push_event (event_id=<optimized out>, event=event@entry=0x20000350) at ./src/trace/trace.c:27
#6  0x0802c406 in trace_push_event (event=0x20000350, event_id=<optimized out>) at ./src/trace/trace.c:21
#7  trace (event_id=event_id@entry=0 '\000') at ./src/trace/trace.c:43
#8  0x080229a4 in panic_hook (reason=0x8053bf4 "BusFault") at ./src/main.c:71
#9  0x08001aa4 in chSysHalt (reason=<optimized out>) at ../lib/ChibiOS/os/rt/src/chsys.c:209
#10 <signal handler called>
#11 0x20012c44 in SDU1 ()
#12 0x08080008 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

(gdb) p ch
$1 = {rlist = {queue = {next = 0x20009a60 <ch_idle_thread_wa+288>, prev = 0x20009a60 <ch_idle_thread_wa+288>}, prio = 0, ctx = {sp = 0x0}, newer = 0x20009af8 <ch+56>, 
    older = 0x200216a8 <shell_spawn::shell_wa+2320>, current = 0x200216a8 <shell_spawn::shell_wa+2320>}, vtlist = {next = 0x2001b070 <uavcan_node_start::thread_wa+7128>, 
    prev = 0x2001264c <wa_lwip_thread+3916>, delta = 4294967295, systime = 21402}, dbg = {panic_msg = 0x805151c "SV#6", isr_cnt = 0, lock_cnt = 0}, mainthread = {queue = {
      next = 0x20009a60 <ch_idle_thread_wa+288>, prev = 0x20009ac0 <ch>}, prio = 128, ctx = {sp = 0x20000ee4}, newer = 0x20009a60 <ch_idle_thread_wa+288>, 
    older = 0x20009ac0 <ch>, name = 0x8051688 <ch_debug> "main", wabase = 0x20000400, state = 8 '\b', flags = 0 '\000', refs = 1 '\001', ticks = 5 '\005', time = 162, 
    u = {rdymsg = -1, exitcode = -1, wtobjp = 0xffffffff, wttrp = 0xffffffff, sentmsg = -1, wtsemp = 0xffffffff, wtmtxp = 0xffffffff, ewmask = 4294967295}, waiting = {
      next = 0x20009b24 <ch+100>}, msgqueue = {next = 0x20009b28 <ch+104>, prev = 0x20009b28 <ch+104>}, epending = 0, mtxlist = 0x0, realprio = 128, mpool = 0x0, stats = {
      best = 912, worst = 703570, last = 1063, n = 1645, cumulative = 5180112}}, tm = {offset = 0}, kernel_stats = {n_irq = 62245, n_ctxswc = 81615, m_crit_thd = {
      best = 141, worst = 196472, last = 852, n = 157869, cumulative = 77083665}, m_crit_isr = {best = 145, worst = 1826, last = 719538785, n = 73922, 
      cumulative = 24538927}}}
antoinealb commented 5 years ago

I don't think the way you are defining the end of the section is right because the linker is free to reorder symbols so your end might not be the real end.

Can you maybe force the last entry to be NULL NULL by placing that in a special var and placing that var at the end using linker.

msplr commented 5 years ago

I don't think the way you are defining the end of the section is right because the linker is free to reorder symbols so your end might not be the real end.

I changed the linkerscript. now the NULL entry is in the same section but forced to the end by sorting.

msplr commented 5 years ago

Hardware panics when I launch the USB shell

Ok weird, it worked in QEMU. I will debug this later then...

msplr commented 5 years ago

For some reason the symbols didn't land in the right section and I don't really understand why. Removing the const qualifier from the ShellCommand entries solved the problem.

msplr commented 5 years ago

Works on hardware.

antoinealb commented 5 years ago

Thats weird, but congrats on fixing it. Also travis appears to be down for now :/

SyrianSpock commented 5 years ago

LGTM, ready to merge when Travis is happy :pray: