Improve OT_JOB + auto-init improved by biboc · Pull Request #4 · jia200x/RIOT

biboc · 2017-05-29T13:16:51Z

~~Remove OT_JOB~~
Auto-init ifconfig with panid and channel (editable in Makefile) and start thread protocol
Address @aabadie comments

aabadie · 2017-05-29T13:18:43Z

pkg/openthread/contrib/platform_alarm.c

        msg_send(&ot_alarm_msg, thread_getpid());
    }
    else {
+        int dt;


Maybe int dt = aDt* ...

aabadie · 2017-05-29T13:20:52Z

pkg/openthread/contrib/platform_radio.c

-
-    return !sDisabled;
+    netopt_state_t state = _get_state();
+    if (state == NETOPT_STATE_OFF || state == NETOPT_STATE_SLEEP) {


return !(state == NETOPT_STATE_OFF || state == NETOPT_STATE_SLEEP) ?

No I prefer to do it in 5 lines, it is more understandable

aabadie · 2017-05-29T13:26:51Z

Looks good to me. Can you merge it @jia200x ?

jia200x · 2017-05-29T14:44:28Z

Good!
The only thing I'm not sure about if it's a good idea to remove the OT_JOB functions (as commented in the openthread_core PR)

What do you think?

biboc · 2017-05-29T15:22:55Z

I see what you mean. Actually, it is useful because all OpenThread functions become available from any other thread and it will use the right OpenThread instance.
I thought it was only for netdev functions.
I mean, to change panid or any other setting, we have to use OpenThread function, I'll add an example in main.c, let me change my PR to see what I mean.

However, we need an unlimited number of ARGS instead of one job context (context is arg?)

biboc · 2017-05-29T15:52:55Z

I would have suggested to continue with RIOT concept and use gnrc_netapi_set/get functions to communicate with openthread stack but it will miss some functionalities so I understand why you suggested OT_JOB finally.

I think OT_JOB needs to more flexible so any kind of function can be called. At the moment, only function like (otInstance*, void*) can be called and no variable is returned so it is only for setter and printer.
If I need to know the channel in the thread main, I currently can't.

biboc · 2017-05-29T17:30:12Z

In fact, what you did in main.c is what we needed! It's kind of wrapper functions!
Let's move these function in openthread contrib folder.

biboc · 2017-05-29T18:43:06Z

In fact, there is a way to get the information back to the thread that call the OT_JOB: with context! It can be used for an arg or an answer. Good job @jia200x !
I'll edit my PR

jia200x · 2017-05-29T19:26:43Z

In fact, there is a way to get the information back to the thread that call the OT_JOB: with context! It can be used for an arg or an answer. Good job @jia200x !

Thanks :). That was the idea behind OT_JOB. It's possible to pass args and read returns with the context variable (maybe this can be improved though).

In the beginning I implemented IPC commands like netdev but I decided to go into the OT_JOB direction because ot API commands might change over time (also, it allows direct calls to otApi commands).

biboc · 2017-05-30T07:37:19Z

@jia200x, tell me what you think about how I improved your OT_JOB
Btw, I'll do all command I think we need to access from RIOT (see in otJobs array)

@aabadie

…start thread protocol Address @aabadie comments Improve OT_JOB

biboc · 2017-05-30T15:07:28Z

Done, tell me what you think.
I tried all functions and they work. The only thing is that you need to be careful about the type of argument answer

biboc · 2017-06-01T08:21:22Z

Guys? @jia200x @aabadie

aabadie · 2017-06-01T12:46:45Z

Will do some comments

aabadie

Very good job @biboc. I have a few comments though. This goes into the right direction !

aabadie · 2017-06-01T12:47:19Z

pkg/openthread/contrib/netdev/openthread_netdev.c

+uint8_t ot_call_job(char* name, void *arg, void* answer) {
    ot_job_t _job;

-    _job.function = job;


no need for an underscore

aabadie · 2017-06-01T12:48:31Z

pkg/openthread/contrib/netdev/openthread_netdev.c

    diagInit(sInstance);
 #endif

+    /* Init default parameters */


Stupid question: would it make sense to add these init calls in the openthread_bootstrap function ?

In bootstrap, sInstance is not init so we can't

ok, that's fine for now

aabadie · 2017-06-01T12:49:33Z

pkg/openthread/contrib/platform_functions_wrapper.c

+typedef struct {
+    const char *mName;                              /**< A pointer to the job name string. */
+    OT_JOB (*mFunction)(otInstance*, void*, void*);    /**< function to be called when executing job */
+} ot_job_command_t;


why not simply use ot_command_t ?

and rename OT_JOB to OT_COMMAND ?

aabadie · 2017-06-01T12:50:13Z

pkg/openthread/contrib/platform_functions_wrapper.c

+            res = 1;
+        }
+    } else {
+        printf("ERROR: ot_exec_job needs to run in OpenThread thread\n");


aabadie · 2017-06-01T12:50:50Z

pkg/openthread/contrib/platform_functions_wrapper.c

+void output_bytes(const char* name, const uint8_t *aBytes, uint8_t aLength)
+{
+    DEBUG("%s: ", name);
+    for (int i = 0; i < aLength; i++)


for () { [...] }

aabadie · 2017-06-01T12:58:57Z

tests/openthread/Makefile

 # If no BOARD is found in the environment, use this default:
 BOARD ?= samr21-xpro

+BOARD_WHITELIST := samr21-xpro iotlab-m3 fox iotlab-a8-m3


would be great if we could add support to other 802.15.4 radios.

Sure, let's merge this PR and people will add other 802.15.4 radios

aabadie · 2017-06-01T12:59:42Z

tests/openthread/main.c

 {
+    printf("Get PANID\n");
+    uint16_t panid = 0;
+    uint8_t res = ot_call_job("panid", NULL, (void*)&panid);


I would prefer to rename ot_call_job to ot_call_command. It makes more sense to me and looks more intuitive.

biboc · 2017-06-01T13:45:27Z

Done @aabadie

aabadie · 2017-06-01T13:51:20Z

pkg/openthread/contrib/netdev/openthread_netdev.c


-    _job.function = job;
-    _job.context = context;
+uint8_t ot_call_command(char* name, void *arg, void* answer) {


maybe change name to command

aabadie · 2017-06-01T13:52:27Z

pkg/openthread/contrib/netdev/openthread_netdev.c

+uint8_t ot_call_command(char* name, void *arg, void* answer) {
+    ot_job_t job;
+
+    job.mName = name;


since job is defined in RIOT contrib code, use attribute that matches the RIOT code style:
job.command, job.arg, job.answer

aabadie · 2017-06-01T13:54:18Z

pkg/openthread/contrib/platform_functions_wrapper.c

+ * @brief   Struct containing an OpenThread job command
+ */
+typedef struct {
+    const char *mName;                              /**< A pointer to the job name string. */


use const char *name, align doxygen

aabadie · 2017-06-01T13:55:22Z

pkg/openthread/contrib/platform_functions_wrapper.c

+
+OT_COMMAND ot_eui64(otInstance* ot_instance, void* arg, void* answer) {
+    if (answer != NULL) {
+        otExtAddress extAddress;


s/extAddress/address/

same here (sorry)

aabadie · 2017-06-01T13:55:50Z

pkg/openthread/contrib/platform_functions_wrapper.c

+
+OT_COMMAND ot_mode(otInstance* ot_instance, void* arg, void* answer) {
+    if (arg != NULL) {
+        otLinkModeConfig linkMode;


s/linkMode/link_mode

well, no leave it like this

biboc · 2017-06-01T14:02:19Z

Ok @aabadie, done
Your suggested changes make sense

jia200x · 2017-06-01T14:49:55Z

@biboc @aabadie Hi guys

Sorry for the late response, I was busy these days due to some IoT demos for the ITC (www.itcdigital.cl)

@biboc thanks a lot for the adoption of OT_COMMANDs. Awesome work!

jia200x · 2017-06-01T14:50:37Z

if everything is OK, I will merge it. Let me know.

Cheers!

aabadie · 2017-06-01T14:54:21Z

if everything is OK, I will merge it. Let me know.
Yes, go ahead !

biboc · 2017-06-01T14:55:05Z

Great!

jia200x · 2017-06-01T15:06:11Z

Merged!

The test randomly fails on `native` due to timers being not accurate but it cannot be otherwise. So better disable it than raising fake errors. main(): This is RIOT! (Version: buildtest) Testing generic evtimer This should list 2 items ev #1 offset=1000 ev #2 offset=500 This should list 4 items ev #1 offset=659 ev #2 offset=341 ev #3 offset=500 ev #4 offset=2454 Are the reception times of all 4 msgs close to the supposed values? At 662 ms received msg 0: "#2 supposed to be 659" At 1009 ms received msg 1: "#0 supposed to be 1000" At 1511 ms received msg 2: "#1 supposed to be 1500" Traceback (most recent call last): File "/tmp/dwq.0.3125418833043728/ef3af88c4b3615788b164464a437df5c/tests/evtimer_msg/tests/01-run.py", line 33, in <module> sys.exit(run(testfunc)) File "/tmp/dwq.0.3125418833043728/ef3af88c4b3615788b164464a437df5c/dist/pythonlibs/testrunner/__init__.py", line 29, in run testfunc(child) File "/tmp/dwq.0.3125418833043728/ef3af88c4b3615788b164464a437df5c/tests/evtimer_msg/tests/01-run.py", line 26, in testfunc assert(actual in range(expected - ACCEPTED_ERROR, expected + ACCEPTED_ERROR)) AssertionError

18392: drivers/servo: reimplement with high level interface r=benpicco a=maribu ### Contribution description The previous servo driver didn't provide any benefit over using PWM directly, as users controlled the servo in terms of PWM duty cycles. This changes the interface to provide a high level interface that abstracts the gory PWM details. In addition, a SAUL layer and auto-initialization is provided. ### Testing procedure The test application provides access to the servo driver via the `saul` shell command. ``` > saul 2022-08-02 22:12:31,826 # saul 2022-08-02 22:12:31,827 # ID Class Name 2022-08-02 22:12:31,830 # #0 ACT_SWITCH LD1(green) 2022-08-02 22:12:31,832 # #1 ACT_SWITCH LD2(blue) 2022-08-02 22:12:31,834 # #2 ACT_SWITCH LD3(red) 2022-08-02 22:12:31,837 # #3 SENSE_BTN B1(User button) 2022-08-02 22:12:31,838 # #4 ACT_SERVO servo > saul write 4 0 2022-08-02 22:12:41,443 # saul write 4 0 2022-08-02 22:12:41,445 # Writing to device #4 - servo 2022-08-02 22:12:41,447 # Data: 0 2022-08-02 22:12:41,450 # [servo] setting 0 to 2949 (0 / 255) 2022-08-02 22:12:41,453 # data successfully written to device #4 > saul write 4 256 2022-08-02 22:12:45,343 # saul write 4 256 2022-08-02 22:12:45,346 # Writing to device #4 - servo 2022-08-02 22:12:45,347 # Data: 256 2022-08-02 22:12:45,351 # [servo] setting 0 to 6865 (255 / 255) 2022-08-02 22:12:45,354 # data successfully written to device #4 ``` Each write resulted in the MG90S servo that I connected to move to the corresponding position. ### Issues/PRs references Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>

biboc mentioned this pull request May 29, 2017

pkg/openthread: core and tests RIOT-OS/RIOT#7082

Merged

aabadie reviewed May 29, 2017

View reviewed changes

biboc force-pushed the openthread-core-2 branch from ec7ac90 to 1cbc34d Compare May 29, 2017 13:19

aabadie reviewed May 29, 2017

View reviewed changes

biboc force-pushed the openthread-core-2 branch from 1cbc34d to d2699ad Compare May 30, 2017 07:33

biboc changed the title ~~Remove OT_JOB + auto-init improved~~ Improve OT_JOB + auto-init improved May 30, 2017

Auto-init ifconfig with panid and channel (editable in Makefile) and …

e765b5c

…start thread protocol Address @aabadie comments Improve OT_JOB

biboc force-pushed the openthread-core-2 branch from d2699ad to e765b5c Compare May 30, 2017 14:58

Merge branch 'openthread_core' into openthread-core-2

3d92093

biboc force-pushed the openthread-core-2 branch from 4a263c3 to c67ff4b Compare May 30, 2017 15:58

Fix ot_job return value and radio state

0b4334e

biboc force-pushed the openthread-core-2 branch from c67ff4b to 0b4334e Compare May 31, 2017 09:27

aabadie requested changes Jun 1, 2017

View reviewed changes

biboc force-pushed the openthread-core-2 branch from b8ae9b1 to 69973ce Compare June 1, 2017 13:49

aabadie reviewed Jun 1, 2017

View reviewed changes

Move OT JOB to OT COMMAND

0256eff

biboc force-pushed the openthread-core-2 branch from 69973ce to 0256eff Compare June 1, 2017 14:01

jia200x merged commit fbea41a into jia200x:openthread_core Jun 1, 2017

Conversation

biboc commented May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 29, 2017

Uh oh!

jia200x commented May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

biboc commented May 29, 2017

Uh oh!

biboc commented May 29, 2017

Uh oh!

biboc commented May 29, 2017

Uh oh!

biboc commented May 29, 2017

Uh oh!

jia200x commented May 29, 2017

Uh oh!

biboc commented May 30, 2017

Uh oh!

biboc commented May 30, 2017

Uh oh!

biboc commented Jun 1, 2017

Uh oh!

aabadie commented Jun 1, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biboc commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biboc commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jia200x commented Jun 1, 2017

Uh oh!

jia200x commented Jun 1, 2017

biboc commented May 29, 2017 •

edited

Loading

jia200x commented May 29, 2017 •

edited

Loading

biboc commented Jun 1, 2017 •

edited

Loading