macros: deleted macros SECONDS(), MSEC(), USEC() #13912

maxxlife · 2019-02-27T19:27:34Z

Using help of the community, we decided to delete these macros SECONDS(), MSEC() and USEC() and replace them with K_MSEC() and K_SECONDS()

Fixes: #7817

Signed-off-by: Maksim Masalski maxxliferobot@gmail.com

andyross

Nitpickery: first, we're supposed to be namespacing our macros, so these should probably start with Z_ as you're adding new ones.

And as far as the name goes "x_PER_y" sounds like a single scalar conversion factor (which we already have) where this is a macro doing the conversion itself. Maybe "Z_{M,U,}SEC_TO_TICKS()" would be better names?

But not worth fighting over. This usage seems to be fine to my eyes given the complaint in the bug.

maxxlife · 2019-02-27T19:52:42Z

Nitpickery: first, we're supposed to be namespacing our macros, so these should probably start with Z_ as you're adding new ones.

And as far as the name goes "x_PER_y" sounds like a single scalar conversion factor (which we already have) where this is a macro doing the conversion itself. Maybe "Z_{M,U,}SEC_TO_TICKS()" would be better names?

But not worth fighting over. This usage seems to be fine to my eyes given the complaint in the bug.

We have 6 more people, so they can say something about names too.

codecov-io · 2019-02-27T20:10:53Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d51ee67). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #13912   +/-   ##
=========================================
  Coverage          ?   52.29%           
=========================================
  Files             ?      307           
  Lines             ?    45452           
  Branches          ?    10519           
=========================================
  Hits              ?    23771           
  Misses            ?    16887           
  Partials          ?     4794

Impacted Files	Coverage Δ
include/sys_clock.h	`100% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d51ee67...9fc0546. Read the comment docs.

andrewboie · 2019-02-27T20:44:14Z

Substance of this patch looks good.
However I agree with @andyross, let's add either Z_ prefix (if these are intended to be private to the kernel) or K_ prefix (if these are intended to be application facing) to these new macros.

maxxlife · 2019-02-27T20:46:48Z

Substance of this patch looks good.
However I agree with @andyross, let's add either Z_ prefix (if these are intended to be private to the kernel) or K_ prefix (if these are intended to be application facing) to these new macros.

Make like Z_{M,U,}SEC_TO_TICKS(), right?

By the way, some Shippable errors, can't understand what is wrong.

andrewboie · 2019-02-27T20:51:29Z

Make like Z_{M,U,}SEC_TO_TICKS(), right?

Well, it depends on whether the API is private to the kernel, or public to applications which determines whether to use the Z_ or K_ prefix, but yes that's what we had in mind.

To me the intention behind these macros appears to be public-facing, so K_ would be appropriate.

ulfalizer · 2019-02-27T21:41:38Z

If we start namespacing things with z_/Z_, then it might be a good idea to make it clear in the documentation that it's an ongoing thing. Otherwise, people are gonna think the prefixed stuff has some special significance compared to the other stuff.

pfalcon · 2019-02-28T10:32:06Z

@maxxlife, Thanks for picking this up! I agree with comments of @andyross and @andrewboie, to summarize, K_{M,U,}SEC_TO_TICKS() should be the best names for now. (Deciding on (consistent) prefixes is the ongoing matter, but glad that we're reached consensus that they're needed, whatever they are.)

maxxlife · 2019-02-28T10:38:28Z

@maxxlife, Thanks for picking this up! I agree with comments of @andyross and @andrewboie, to summarize, K_{M,U,}SEC_TO_TICKS() should be the best names for now. (Deciding on (consistent) prefixes is the ongoing matter, but glad that we're reached consensus that they're needed, whatever they are.)

Thanks. Do you know how to solve problem with Shippable?

pfalcon · 2019-02-28T10:51:11Z

Thanks. Do you know how to solve problem with Shippable?

Well, just click thru the web UI to get to the error messages. E.g.
https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/35722/1/tests shows that not all cases were converted:

FAILED: ccache /opt/sdk/zephyr-sdk-0.9.5/sysroots/x86_64-pokysdk-linux/usr/bin/arm-zephyr-eabi/arm-zephyr-eabi-gcc -DBUILD_VERSION=v1.14.0-rc1-623-g327632cfc70b -DKERNEL -DSTM32F401xE -DUSE_FULL_LL_DRIVER -DUSE_HAL_DRIVER -D_FORTIFY_SOURCE=2 -D__ZEPHYR__=1 -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/kernel/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/arch/arm/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/include/drivers -Izephyr/include/generated -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/soc/arm/st_stm32/stm32f4 -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/lib/libc/minimal/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/cmsis/Include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/st/stm32cube/stm32f4xx/soc -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/st/stm32cube/stm32f4xx/drivers/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/st/stm32cube/stm32f4xx/drivers/include/Legacy -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/subsys/testsuite/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/subsys/testsuite/ztest/include -isystem /opt/sdk/zephyr-sdk-0.9.5/sysroots/x86_64-pokysdk-linux/usr/lib/arm-zephyr-eabi/gcc/arm-zephyr-eabi/6.2.0/include -isystem /opt/sdk/zephyr-sdk-0.9.5/sysroots/x86_64-pokysdk-linux/usr/lib/arm-zephyr-eabi/gcc/arm-zephyr-eabi/6.2.0/include-fixed -Os -nostdinc -g -Wall -Wformat -Wformat-security -Wno-format-zero-length -imacros /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/sanity-out/96b_carbon/tests/kernel/mem_protect/mem_protect/kernel.memory_protection/zephyr/include/generated/autoconf.h -ffreestanding -Wno-main -fno-common --sysroot /opt/sdk/zephyr-sdk-0.9.5/sysroots/armv5-zephyr-eabi/usr -mthumb -mcpu=cortex-m4 -Werror -Wno-deprecated-declarations -fno-asynchronous-unwind-tables -fno-pie -fno-pic -fno-strict-overflow -Wno-pointer-sign -Wno-unused-but-set-variable -fno-reorder-functions -fno-defer-pop -Werror=implicit-int -Wpointer-arith -ffunction-sections -fdata-sections -mabi=aapcs -march=armv7e-m -std=c99 -MD -MT CMakeFiles/app.dir/src/kobject.c.obj -MF CMakeFiles/app.dir/src/kobject.c.obj.d -o CMakeFiles/app.dir/src/kobject.c.obj   -c /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/tests/kernel/mem_protect/mem_protect/src/kobject.c
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/tests/kernel/mem_protect/mem_protect/src/kobject.c: In function test_kobject_revoke_access:
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/tests/kernel/mem_protect/mem_protect/src/kobject.c:161:24: error: implicit declaration of function MSEC [-Werror=implicit-function-declaration]
  k_sem_take(&sync_sem, MSEC(100));
                        ^~~~

maxxlife · 2019-02-28T10:57:24Z

Thanks. Do you know how to solve problem with Shippable?

Well, just click thru the web UI to get to the error messages. E.g.
https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/35722/1/tests shows that not all cases were converted:

FAILED: ccache /opt/sdk/zephyr-sdk-0.9.5/sysroots/x86_64-pokysdk-linux/usr/bin/arm-zephyr-eabi/arm-zephyr-eabi-gcc -DBUILD_VERSION=v1.14.0-rc1-623-g327632cfc70b -DKERNEL -DSTM32F401xE -DUSE_FULL_LL_DRIVER -DUSE_HAL_DRIVER -D_FORTIFY_SOURCE=2 -D__ZEPHYR__=1 -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/kernel/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/arch/arm/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/include/drivers -Izephyr/include/generated -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/soc/arm/st_stm32/stm32f4 -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/lib/libc/minimal/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/cmsis/Include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/st/stm32cube/stm32f4xx/soc -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/st/stm32cube/stm32f4xx/drivers/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/ext/hal/st/stm32cube/stm32f4xx/drivers/include/Legacy -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/subsys/testsuite/include -I/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/subsys/testsuite/ztest/include -isystem /opt/sdk/zephyr-sdk-0.9.5/sysroots/x86_64-pokysdk-linux/usr/lib/arm-zephyr-eabi/gcc/arm-zephyr-eabi/6.2.0/include -isystem /opt/sdk/zephyr-sdk-0.9.5/sysroots/x86_64-pokysdk-linux/usr/lib/arm-zephyr-eabi/gcc/arm-zephyr-eabi/6.2.0/include-fixed -Os -nostdinc -g -Wall -Wformat -Wformat-security -Wno-format-zero-length -imacros /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/sanity-out/96b_carbon/tests/kernel/mem_protect/mem_protect/kernel.memory_protection/zephyr/include/generated/autoconf.h -ffreestanding -Wno-main -fno-common --sysroot /opt/sdk/zephyr-sdk-0.9.5/sysroots/armv5-zephyr-eabi/usr -mthumb -mcpu=cortex-m4 -Werror -Wno-deprecated-declarations -fno-asynchronous-unwind-tables -fno-pie -fno-pic -fno-strict-overflow -Wno-pointer-sign -Wno-unused-but-set-variable -fno-reorder-functions -fno-defer-pop -Werror=implicit-int -Wpointer-arith -ffunction-sections -fdata-sections -mabi=aapcs -march=armv7e-m -std=c99 -MD -MT CMakeFiles/app.dir/src/kobject.c.obj -MF CMakeFiles/app.dir/src/kobject.c.obj.d -o CMakeFiles/app.dir/src/kobject.c.obj   -c /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/tests/kernel/mem_protect/mem_protect/src/kobject.c
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/tests/kernel/mem_protect/mem_protect/src/kobject.c: In function test_kobject_revoke_access:
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/tests/kernel/mem_protect/mem_protect/src/kobject.c:161:24: error: implicit declaration of function MSEC [-Werror=implicit-function-declaration]
  k_sem_take(&sync_sem, MSEC(100));
                        ^~~~

Oh, now I will keep in mind. Never worked with Shippable before. In the evening (~ after 5 hours) will fix it.

andrewboie · 2019-02-28T17:32:44Z

If we start namespacing things with z_/Z_, then it might be a good idea to make it clear in the documentation that it's an ongoing thing. Otherwise, people are gonna think the prefixed stuff has some special significance compared to the other stuff.

See #9882
I thought this was documented somewhere though @nashif any idea?

jukkar

Should we actually get rid of the ticks version as I thought that ticks != ms and to me it looks like all the usage patched by this PR expects milliseconds?

subsys/net/l2/ethernet/gptp/gptp_messages.c

tests/kernel/mem_protect/mem_protect/src/kobject.c

jukkar

Let me give -1 for time being until the ticks vs ms is clarified.

andyross · 2019-03-01T16:33:10Z

Heh, @maxxlife, you may have bitten off a design issue. In theory all (really, all) use of "ticks" should be internal to the kernel: the timeout system, IPC timeouts, etc... We don't expose ticks as an API anymore. So if you see ones outside the kernel directory, they're probably wrong. And if you don't see any inside the kernel, then the proper patch becomes to remove these macros entirely.

maxxlife · 2019-03-01T19:35:22Z

Heh, @maxxlife, you may have bitten off a design issue. In theory all (really, all) use of "ticks" should be internal to the kernel: the timeout system, IPC timeouts, etc... We don't expose ticks as an API anymore. So if you see ones outside the kernel directory, they're probably wrong. And if you don't see any inside the kernel, then the proper patch becomes to remove these macros entirely.

@andyross I checked code
Can we just get rid of these macros K_SEC_TO_TICKS(), K_MSEC_TO_TICKS(), K_USEC_TO_TICKS()?
I think in all places is suitable to use K_MSEC or K_SECONDS.

andyross

Code looks fine to me, but can you squash these? Really this should be just one patch.

maxxlife · 2019-03-01T21:22:11Z

Code looks fine to me, but can you squash these? Really this should be just one patch.

@andyross Delete all previous commits, am I right?

andyross · 2019-03-01T21:25:59Z

Right. Submit just one patch with one commit message.

pfalcon · 2019-03-01T21:30:19Z

To elaborate: git rebase -i master, then follow instructions. Rather than "Delete all previous commits", it's "Replace all previous commits with a new one which combines all previous".

maxxlife · 2019-03-01T21:52:32Z

To elaborate: git rebase -i master, then follow instructions. Rather than "Delete all previous commits", it's "Replace all previous commits with a new one which combines all previous".

@pfalcon It seems I made it.

pfalcon · 2019-03-01T21:55:19Z

What exactly? I still see 5 commits in this PR. After interactive rebase using "squash" operations, you need to git push --force ... this branch.

Changed everywhere these macros to the K_MSEC(), K_SECONDS() Signed-off-by: Maksim Masalski <maxxliferobot@gmail.com>

maxxlife · 2019-03-01T23:01:24Z

What exactly? I still see 5 commits in this PR. After interactive rebase using "squash" operations, you need to git push --force ... this branch.

The first step is always the hardest, but finally I made it!

pfalcon · 2019-03-02T12:02:03Z

Looks good, thanks!

tests/kernel/semaphore/semaphore/src/main.c

andyross

Code looks good to me now, though I'll admit I'm a little creeped out at how many spots are being changed in the test code. A default kconfig is going to have a 100 Hz tick rate, and the ms numbers were now getting from those macros are 10x higher than what we had, even if they clearly reflect the intent of the code.

If everything passes, I guess I'm fine. But I'd be prepared for some rough waters ahead as things restabilize. Surely something's going to break somewhere.

tests/kernel/semaphore/semaphore/src/main.c

My concern turned out to be just badly named variables

maxxlife requested review from andrewboie, andyross, jhedberg, jukkar, nashif, pfalcon and tbursztyka as code owners February 27, 2019 19:27

andyross approved these changes Feb 27, 2019

View reviewed changes

andrewboie approved these changes Feb 28, 2019

View reviewed changes

jukkar reviewed Mar 1, 2019

View reviewed changes

subsys/net/l2/ethernet/gptp/gptp_messages.c Outdated Show resolved Hide resolved

tests/kernel/mem_protect/mem_protect/src/kobject.c Outdated Show resolved Hide resolved

jukkar requested changes Mar 1, 2019

View reviewed changes

andyross requested changes Mar 1, 2019

View reviewed changes

maxxlife force-pushed the macros_names_7817 branch from 69586d5 to be7293c Compare March 1, 2019 21:51

maxxlife changed the title ~~macros: changed names of SECONDS(), MSEC(), USEC()~~ macros: deleted macros SECONDS(), MSEC(), USEC() Mar 1, 2019

maxxlife force-pushed the macros_names_7817 branch 3 times, most recently from 23c8d79 to 08354ba Compare March 1, 2019 22:53

macros: deleted macros SECONDS(), MSEC(), USEC()

9fc0546

Changed everywhere these macros to the K_MSEC(), K_SECONDS() Signed-off-by: Maksim Masalski <maxxliferobot@gmail.com>

maxxlife force-pushed the macros_names_7817 branch from 08354ba to 9fc0546 Compare March 1, 2019 22:59

pfalcon approved these changes Mar 2, 2019

View reviewed changes

jhedberg previously requested changes Mar 2, 2019

View reviewed changes

tests/kernel/semaphore/semaphore/src/main.c Show resolved Hide resolved

tests/kernel/semaphore/semaphore/src/main.c Show resolved Hide resolved

nashif requested review from andyross and jukkar March 2, 2019 12:28

andyross approved these changes Mar 2, 2019

View reviewed changes

tests/kernel/semaphore/semaphore/src/main.c Show resolved Hide resolved

jukkar approved these changes Mar 4, 2019

View reviewed changes

nashif approved these changes Mar 5, 2019

View reviewed changes

nashif merged commit b324f35 into zephyrproject-rtos:master Mar 5, 2019

maxxlife deleted the macros_names_7817 branch May 2, 2019 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

macros: deleted macros SECONDS(), MSEC(), USEC() #13912

macros: deleted macros SECONDS(), MSEC(), USEC() #13912

maxxlife commented Feb 27, 2019 •

edited

Loading

andyross left a comment

maxxlife commented Feb 27, 2019 •

edited

Loading

codecov-io commented Feb 27, 2019 •

edited

Loading

andrewboie commented Feb 27, 2019

maxxlife commented Feb 27, 2019

andrewboie commented Feb 27, 2019

ulfalizer commented Feb 27, 2019

pfalcon commented Feb 28, 2019

maxxlife commented Feb 28, 2019

pfalcon commented Feb 28, 2019

maxxlife commented Feb 28, 2019 •

edited

Loading

andrewboie commented Feb 28, 2019

jukkar left a comment

jukkar left a comment

andyross commented Mar 1, 2019

maxxlife commented Mar 1, 2019 •

edited

Loading

andyross left a comment

maxxlife commented Mar 1, 2019

andyross commented Mar 1, 2019

pfalcon commented Mar 1, 2019

maxxlife commented Mar 1, 2019 •

edited

Loading

pfalcon commented Mar 1, 2019

maxxlife commented Mar 1, 2019

pfalcon commented Mar 2, 2019

andyross left a comment

macros: deleted macros SECONDS(), MSEC(), USEC() #13912

macros: deleted macros SECONDS(), MSEC(), USEC() #13912

Conversation

maxxlife commented Feb 27, 2019 • edited Loading

andyross left a comment

Choose a reason for hiding this comment

maxxlife commented Feb 27, 2019 • edited Loading

codecov-io commented Feb 27, 2019 • edited Loading

Codecov Report

andrewboie commented Feb 27, 2019

maxxlife commented Feb 27, 2019

andrewboie commented Feb 27, 2019

ulfalizer commented Feb 27, 2019

pfalcon commented Feb 28, 2019

maxxlife commented Feb 28, 2019

pfalcon commented Feb 28, 2019

maxxlife commented Feb 28, 2019 • edited Loading

andrewboie commented Feb 28, 2019

jukkar left a comment

Choose a reason for hiding this comment

jukkar left a comment

Choose a reason for hiding this comment

andyross commented Mar 1, 2019

maxxlife commented Mar 1, 2019 • edited Loading

andyross left a comment

Choose a reason for hiding this comment

maxxlife commented Mar 1, 2019

andyross commented Mar 1, 2019

pfalcon commented Mar 1, 2019

maxxlife commented Mar 1, 2019 • edited Loading

pfalcon commented Mar 1, 2019

maxxlife commented Mar 1, 2019

pfalcon commented Mar 2, 2019

andyross left a comment

Choose a reason for hiding this comment

maxxlife commented Feb 27, 2019 •

edited

Loading

maxxlife commented Feb 27, 2019 •

edited

Loading

codecov-io commented Feb 27, 2019 •

edited

Loading

maxxlife commented Feb 28, 2019 •

edited

Loading

maxxlife commented Mar 1, 2019 •

edited

Loading

maxxlife commented Mar 1, 2019 •

edited

Loading