Add names to system thread #8612

vidavidorra · 2018-11-01T13:02:38Z

Description

Add names to the PPP and cellular FSM threads. This would make those names visible when debugging threads, e.g. as described in Tracking memory usage with Mbed OS

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

kjbracey · 2018-11-01T13:16:49Z

Yes, good - more tools are having thread awareness now, eg pyOCD and error dumps.

While here, mbed_shared_queues.cpp could also do with names for the 2 shared threads - I've noticed them being blank in debuggers. Could you do those at the same time?

cmonr

LGTM

vidavidorra · 2018-11-02T08:27:53Z

@kjbracey-arm Sure. I only found one thread though, here. I can name that shared_event_queue if that's okay.
@cmonr @AnttiKauppila @kjbracey-arm Should I add one commit to this PR with the name of that thread added?

SeppoTakalo

I'm OK with the change.

However, I would propose not to use _thread suffix on threads, as they are...thereads..anyway..

The fsm_queue_thread don't tell anything for me, but how about cellular_fsm_queue

Then event_thread is kind of OK, but we already have 3 event loops(normal, high priority and lwip), so why not specifically lwip_event_loop

SeppoTakalo · 2018-11-02T09:35:55Z

And while we are at it, the shared event queues should be named like shared_event_queue as propsed and the another as shared_highprio_event_queue

kjbracey · 2018-11-02T09:42:39Z

I only found one thread though

That's a template - it's used twice to create the two threads below. You'll have to pass the names as a parameter to do_shared_event_queue_with_thread, can't do strings with templates.

However, I would propose not to use _thread suffix on threads, as they are...thereads..anyway..

Totally agree, but for consistency reasons I note that we do have "idle_thread", "timer_thread" in mbed_rtx_conf.h and "main_thread" in mbed_rtos_rtx.c.

I would favour dropping their suffixes.

kjbracey · 2018-11-02T09:51:04Z

shared_event_queue

Alternative is mbed_event_queue, as that's the function you call to get it - more searchable. Although I do agree shared is more meaningful (and maybe should have been in the name of the function). No strong preference.

vidavidorra · 2018-11-02T10:13:52Z

However, I would propose not to use _thread suffix on threads, as they are...thereads..anyway..

Also agree. I'll change the names accordingly.

main_thread > main
idle_thread > idle
tcpip_thread > tcpip
timer_thread > timer
event_thread > lwip_event_queue
fsm_queue_thread > cellular_fsm_queue

Then event_thread is kind of OK, but we already have 3 event loops(normal, high priority and lwip), so why not specifically lwip_event_loop

Agree, name should specifically say what it is. Why not lwip_event_queue? In the code I see the callback for the event_thread is event_queue (source). I don't have a lot of knowledge what is happening underneath so if loop makes more sense I'll name it loop.

That's a template - it's used twice to create the two threads below. You'll have to pass the names as a parameter to do_shared_event_queue_with_thread, can't do strings with templates.

Should I also include that in the commit?
I'd than add std::string name as argument to do_shared_event_queue_with_thread and add names to the two callers L65 and L72, shared_event_queue and shared_highprio_event_queue respectively?

Another question. Is it useful to prefix all system threads with something like mbed_ or os_ so that it is obvious that certain threads are system/os threads and they can be more easily be extinguished from application/user threads?

Also, since some names are getting a bit longer. Isn't that going to be a problem with maximum lengths of the name. I don't know how they are stored, but thought it was worth to mention here.

kjbracey · 2018-11-02T10:55:04Z

I'd than add std::string name as argument to do_shared_event_queue_with_thread and add names to the two callers L65 and L72, shared_event_queue and shared_highprio_event_queue respectively?

Yes, but just const char *. std::string adds a whole pile of run-time stuff and RAM/heap use you don't want for a simple string constant. (If this was C++17 or whatever, a std::string_view would be a good C++ type, but until then stick with const char *).

Isn't that going to be a problem with maximum lengths of the name.

Apart from a teeny bit of extra ROM use, no. The RTOS doesn't store the names, just stores a pointer to the string literal you pass.

Is it useful to prefix all system threads with something like mbed_ or os_

Possibly, yes. That's one reason to favour mbed_event_queue. os_ could be rtx_, I guess, to be more specific? I don't think you need mbed in front of every component, but there should at least be clear prefixes like lwip_.

lwip_event_thread

That is specifically for PPP, not lwip in general, so I'd say ppp_event_queue or ppp_lwip_event_queue.

Although again, the event_queue suffix isn't adding much if there's only one ppp_lwip thread. May as well just be ppp_lwip.

Remove _thread suffix and rename threads.

vidavidorra · 2018-11-02T12:54:31Z

I've added two commits for this. I was just wondering, is there an easy way to run most of the (unit) tests locally before I make a commit?
Now I just push the commit but it would be great if I can somehow run similar tests than those that are run here, just to be sure the code is tested somewhat before I commit it. Couldn't really find anything about this in the docs.

cmonr · 2018-11-02T14:01:57Z

@vidavidorra It's not the most intuitive, but the docs for the unit tests are in the subdirectroy of Mbed OS: https://github.com/ARMmbed/mbed-os/tree/master/UNITTESTS

vidavidorra · 2018-11-02T15:00:59Z

@cmonr Thanks! Wasn't able to run locally yet but will check that later. As for the tests that are failing here. I saw in the travis log that it fails here I'm not sure why though. Compile was successful but test failed. Can anyone more familiar with the tests etc. have a look at it, because I'm having a hard time finding out why it is failing.

kjbracey · 2018-11-02T13:11:43Z

features/cellular/easy_cellular/CellularConnectionFSM.cpp

@@ -617,7 +617,7 @@ nsapi_error_t CellularConnectionFSM::start_dispatch()
 {
    MBED_ASSERT(!_queue_thread);

-    _queue_thread = new rtos::Thread(osPriorityNormal, 2048, NULL, "fsm_queue_thread");
+    _queue_thread = new rtos::Thread(osPriorityNormal, 2048, NULL, "cellular_fsm_queue");


Would be inclined to cut this down to cellular_fsm

Fine by me 😃 added it.

kjbracey · 2018-11-02T15:15:59Z

Reran travis and it passed - timing-based failure on "simple barrage test". Server load?

SeppoTakalo · 2018-11-02T16:14:26Z

rtos/TARGET_CORTEX/mbed_rtos_rtx.c

@@ -52,7 +52,7 @@ MBED_NORETURN void mbed_rtos_start()
    _main_thread_attr.cb_size = sizeof(_main_obj);
    _main_thread_attr.cb_mem = &_main_obj;
    _main_thread_attr.priority = osPriorityNormal;
-    _main_thread_attr.name = "main_thread";
+    _main_thread_attr.name = "rtx_main";


Main thread should probably keep its name as this is actually where the main() starts.

SeppoTakalo

Looks good, but I would still like to keep the main thread name as it is.. it is not to serve RTX, but actually the one that runs my main() so main_thread is sensible name.

vidavidorra · 2018-11-02T19:31:35Z

Ok, so I'll change that back to main than since we ditched the thread suffixes everywhere.

0xc0170 · 2018-11-06T12:05:56Z

/morph build

mbed-ci · 2018-11-06T17:17:53Z

Build : SUCCESS

Build number : 3557
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8612/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-11-06T18:39:02Z

Exporter Build : SUCCESS

Build number : 3165
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8612/

cmonr · 2018-11-06T20:08:54Z

/morph mbed2-build

mbed-ci · 2018-11-06T22:25:21Z

Test : SUCCESS

Build number : 3339
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8612/3339

jdbruijn added 2 commits November 1, 2018 13:55

feat: Name cellular FSM queue thread

56a12ce

feat: Name PPP thread

1f4f623

cmonr requested a review from a team November 1, 2018 21:17

cmonr added the needs: review label Nov 1, 2018

cmonr approved these changes Nov 1, 2018

View reviewed changes

AnttiKauppila approved these changes Nov 2, 2018

View reviewed changes

SeppoTakalo approved these changes Nov 2, 2018

View reviewed changes

jdbruijn added 2 commits November 2, 2018 13:44

fix: Update thread names

3f635ef

Remove _thread suffix and rename threads.

feat: Add name to shared queue threads

467a7b6

kjbracey reviewed Nov 2, 2018

View reviewed changes

fix: Change cellular FSM thread name

440905a

SeppoTakalo reviewed Nov 2, 2018

View reviewed changes

SeppoTakalo approved these changes Nov 2, 2018

View reviewed changes

fix: Remove rtx from main thread name

2ef82e1

kjbracey approved these changes Nov 5, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Nov 5, 2018

0xc0170 approved these changes Nov 5, 2018

View reviewed changes

0xc0170 added the release-version: 5.11.0-rc1 label Nov 5, 2018

cmonr merged commit 0c2d35f into ARMmbed:master Nov 6, 2018

cmonr removed the needs: CI label Nov 7, 2018

Add names to system thread #8612

Add names to system thread #8612

Uh oh!

Conversation

vidavidorra commented Nov 1, 2018

Description

Pull request type

Uh oh!

kjbracey commented Nov 1, 2018

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

vidavidorra commented Nov 2, 2018

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Nov 2, 2018

Uh oh!

kjbracey commented Nov 2, 2018

Uh oh!

kjbracey commented Nov 2, 2018

Uh oh!

vidavidorra commented Nov 2, 2018

Uh oh!

kjbracey commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vidavidorra commented Nov 2, 2018

Uh oh!

cmonr commented Nov 2, 2018

Uh oh!

vidavidorra commented Nov 2, 2018

Uh oh!

kjbracey Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

vidavidorra Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Nov 2, 2018

Uh oh!

SeppoTakalo Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

vidavidorra commented Nov 2, 2018

Uh oh!

0xc0170 commented Nov 6, 2018

Uh oh!

mbed-ci commented Nov 6, 2018

Build : SUCCESS

Triggering tests

Uh oh!

mbed-ci commented Nov 6, 2018

Exporter Build : SUCCESS

Uh oh!

cmonr commented Nov 6, 2018

Uh oh!

mbed-ci commented Nov 6, 2018

Test : SUCCESS

Uh oh!

Uh oh!

kjbracey commented Nov 2, 2018 •

edited

Loading