Skip to content

core: add an argument to thread_create() (implementation)#856

Merged
Kijewski merged 2 commits intoRIOT-OS:masterfrom
Kijewski:issue-847
Jul 9, 2014
Merged

core: add an argument to thread_create() (implementation)#856
Kijewski merged 2 commits intoRIOT-OS:masterfrom
Kijewski:issue-847

Conversation

@Kijewski
Copy link
Copy Markdown
Contributor

@Kijewski Kijewski commented Mar 4, 2014

See discussion in #847. This PR adds a parameter to thread_create() that is passed to the entry function.

I implemented the additional argument for native and ARM boards. Tested with test_thread_cooperation on native and msba2.

I did not look into how to fix msp430-common's thread_stack_init(), because I don't have a board to test with, anyway. I guess all you have to do is set the last "saved" register in cpu.c#L78 to arg.


Right now this adds a new function instead of changing the current one. The sole reason for that is not to break the current tests. After we have confirmed that the change does work we can work on updating the tests and examples.

I would be happy to see PRs to my branch from kind people that want to help me fix the current examples and tests. -- That is if the discussion shows a consent on @kaspar030's suggestion to change the core function and don't use a legacy wrapper.

@mehlis
Copy link
Copy Markdown
Contributor

mehlis commented Apr 18, 2014

@Kijewski needs rebase

@Kijewski Kijewski modified the milestones: Release 2014.04, Release NEXT MAJOR Apr 18, 2014
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arg

@mehlis
Copy link
Copy Markdown
Contributor

mehlis commented Apr 19, 2014

I like the idea, and the implementation seems to be strait forward.

I want to use this functionality to start a helper thread with its private start parameter, different helper, different task

@LudwigOrtmann @OlegHahm @kaspar030 what do you think?

@Kijewski
Copy link
Copy Markdown
Contributor Author

Yes, but please ignore my last commit [WIP] drop thread_create_ex. It's too complicated for me as one person to fix all thread_create()s. I would rather keep the the helper wrapper, replace all occurrences of thread_create() with thread_create_ex() and then rename thread_create_ex() into thread_create() again.

@Kijewski
Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the meaning of _ex ? extended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, extended. The function is not meant to stick around for long, so I don't think that too much thoughts should be wasted on how to name it.

@mehlis mehlis added the core label Apr 20, 2014
@Kijewski
Copy link
Copy Markdown
Contributor Author

I added the argument passing to msp430-common. Tested with mspsim for telosb.
Now only lpc1768 is left.

@Kijewski
Copy link
Copy Markdown
Contributor Author

@rousselk, could you please test this branch on a real MSP device? Executing hello-world would be enough actually, because the thread entry already uses the function to execute as argument.

@Kijewski
Copy link
Copy Markdown
Contributor Author

@haukepetersen, @OlegHahm I cannot decipher the assembly in cortexm_common and lpc1768. I won't be able to fixup thread_(arch_)stack_init for these CPUs.

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jun 3, 2014

@haukepetersen, you said you were looking into that / providing me information after the meeting two weeks ago. (I am ARM assembler illiterate. :) )

@OlegHahm OlegHahm modified the milestones: Release NEXT MAJOR, FIX ME FIRST Jun 3, 2014
@Kijewski Kijewski mentioned this pull request Jun 3, 2014
@haukepetersen
Copy link
Copy Markdown
Contributor

Yes, true. The solution should be quite straight forwards, in thread_arch_stack_init exchange

/* r0 - r3 */
for (int i = 3; i >= 0; i--) {
    stk--;
    *stk = i;
}

with

/* r1 - r3 */
for (int i = 3; i >= 1; i--) {
    stk--;
    *stk = i;
}
/* r0 -> thread function parameter */
stk--;
stk = (uint32_t)arg;

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jun 6, 2014

@haukepetersen, thanks!

Rebased, and last boards added. Now we need to test it with every supported board.

  • arduino-due
  • avsextrem
  • chronos
  • mbed_lpc1768
  • msb-430
  • msb-430h
  • msba2
  • native
  • pttu
  • qemu-i386
  • redbee-econotag
  • telosb
  • udoo
  • wsn430-v1_3b
  • wsn430-v1_4
  • z1

@OlegHahm
Copy link
Copy Markdown
Member

I'll do next week for the Z1 and msb-430(h).

@Kijewski
Copy link
Copy Markdown
Contributor Author

@OlegHahm, please don't forget to check. ;)

@OlegHahm
Copy link
Copy Markdown
Member

Can you rebase first?

@Kijewski
Copy link
Copy Markdown
Contributor Author

The rebase worked without any conflicts. I don't know why the button was grayed out.

@OlegHahm
Copy link
Copy Markdown
Member

Now Travis is complaining.

@Kijewski
Copy link
Copy Markdown
Contributor Author

Fixed.

@OlegHahm
Copy link
Copy Markdown
Member

Now I forgot to test... Try to think of it next week.

@Kijewski
Copy link
Copy Markdown
Contributor Author

@OlegHahm please don't forget to test. :)

@OlegHahm
Copy link
Copy Markdown
Member

Tested for Z1, MSB-430(h) and redbee-econotag: everything's okay. Tested on IoT-Lab_M3: at least ipc_pingpong example is not working any more.

@OlegHahm
Copy link
Copy Markdown
Member

I modified https://github.com/RIOT-OS/thirdparty_cpu/blob/master/stm32f103rey6/cpu.c#L211 and the following following the cortex_m example to test with IoT-Lab_M3.

@Kijewski
Copy link
Copy Markdown
Contributor Author

I don't know ARM, I just did what @haukepetersen wrote. :-/
Maybe you could only use the first commit and print out the registers in thread_create_entry()?

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jul 2, 2014

@haukepetersen, or @thomaseichinger maybe? Could you maybe look into the problem with IoT-Lab-M3?

@haukepetersen
Copy link
Copy Markdown
Contributor

Yes, we can :-) I dont have a board here - will try to get one tomorrow.

@haukepetersen
Copy link
Copy Markdown
Contributor

Just tested this with the arduino-due, at leas ipc_pingpong and test_vtimer_msg work just fine.

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Jul 4, 2014

Works with #PR1363 version for IoT-Lab_M3, too. I don't think we need to test it for the rest of the platforms: ACK

@mehlis
Copy link
Copy Markdown
Contributor

mehlis commented Jul 6, 2014

@Kijewski PR needs rebase

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jul 8, 2014

Rebased.

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jul 9, 2014

Please second ACK anyone. 👀

@LudwigKnuepfer
Copy link
Copy Markdown
Member

AC

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jul 9, 2014

I guess "AC" is good enough. :)

Kijewski added a commit that referenced this pull request Jul 9, 2014
 core: add an argument to `thread_create()` (implementation)
@Kijewski Kijewski merged commit 7a2f64b into RIOT-OS:master Jul 9, 2014
@LudwigKnuepfer
Copy link
Copy Markdown
Member

K

LudwigKnuepfer pushed a commit to LudwigKnuepfer/RIOT that referenced this pull request Jul 9, 2014
@Kijewski Kijewski deleted the issue-847 branch July 10, 2014 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants