Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isrpipe: split isrpipe_read_timeout to isolate xtimer dependency #11267

Merged
merged 2 commits into from Mar 26, 2019

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Mar 25, 2019

Contribution description

This defines a new 'isrpipe_read_timeout' module that should be used when using
the timeout based function of isrpipe.

This fix the implicit dependency to 'xtimer' that is only needed for the
'_timeout' functions.

It prevents 'stdio_uart' that uses 'isrpipe' to need to depend on xtimer.
This was silently solved at link time for most platforms but not for the
'esp32' for example.

'drivers/at' needed to be updated at the same time to follow the api change.

Testing procedure

In master, the esp32 actually provides warning when compiling some of the applications regarding the missing xtimer symbols. But there are only handled as warnings (#11246)

RIOT_CI_BUILD=1 DOCKER="sudo docker" BOARD=esp32-wroom-32 BUILD_IN_DOCKER=1 make -C tests/pkg_cn-cbor/
...
make" -C /data/riotbuild/riotbase/tests/pkg_cn-cbor/bin/pkg/esp32-wroom-32/cn-cbor/src -f /data/riotbuild/riotbase/pkg/cn-cbor/Makefile.cn-cbor
/data/riotbuild/riotbase/tests/pkg_cn-cbor/bin/esp32-wroom-32/isrpipe.a(isrpipe.o): In function `isrpipe_read':
isrpipe.c:(.text+0x84): warning: undefined reference to `_xtimer_set'
isrpipe.c:(.text+0x88): warning: undefined reference to `xtimer_remove'
isrpipe.c:(.text+0xb2): warning: undefined reference to `_xtimer_set'
/data/riotbuild/riotbase/tests/pkg_cn-cbor/bin/esp32-wroom-32/isrpipe.a(isrpipe.o): In function `isrpipe_read_timeout':

With this pull request there is no reported error anymore. It also removes all the issues with isrpipe.c in #11246

Issues/PRs references

Part of enabling error at link time for esp #11246
It was found while trying to run the test suite on esp32.

@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Mar 25, 2019
@cladmi cladmi added this to the Release 2019.04 milestone Mar 25, 2019
@cladmi cladmi requested review from miri64 and kaspar030 March 25, 2019 16:59
@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 25, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Mar 25, 2019

I am not that used to write module headers and the doxygen so I welcome feedback on this :)

I kept the original license as I was moving the functions around files, tell me if this is wrong.

@cladmi cladmi added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 25, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Mar 25, 2019

Note for me, I noticed a missing word in the first commit message (needs squashing label)

@miri64
Copy link
Member

miri64 commented Mar 26, 2019

Note for me, I noticed a missing word in the first commit message (needs squashing label)

Please feel free to do so.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Except for the module doc I'm fine. The code and documentation of the functions was just copy-pasted.

*/

/**
* @defgroup isr_pipe ISR Pipe
Copy link
Member

Choose a reason for hiding this comment

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

This is just copy-pasted from isrpipe. You have two options here either remove the @defgroup and @brief tag and just do @ingroup isr_pipe or (since you did not explicitly document that a separate module is needed, I would prefer that)

/**
 * @defgroup isr_pipe_read_timeout  Read timeouts with ISR pipe
 * @ingroup  isr_pipe
 * @brief  ISR -> userspace pipe with timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I do the second one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I aligned the arguments to column 17 to match the other ones.
I also updated the C file to match the isr_pipe_read_timeout.

@cladmi cladmi force-pushed the pr/isrpipe/separate_read_timeout branch from 65a0603 to b1f6418 Compare March 26, 2019 19:22
@cladmi
Copy link
Contributor Author

cladmi commented Mar 26, 2019

I forced push to update the commit message then added the other one after.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Rendered doc also looks good:

isr_pipe_read_timeouts

@miri64
Copy link
Member

miri64 commented Mar 26, 2019

Please squash

@cladmi cladmi force-pushed the pr/isrpipe/separate_read_timeout branch from c10c469 to 875bfaa Compare March 26, 2019 19:45
@cladmi
Copy link
Contributor Author

cladmi commented Mar 26, 2019

Squashed, now waiting for the CI queue.

@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 26, 2019
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Apart from some label issues this is what murdock complains about. Please squash immediately

@miri64 miri64 added CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before labels Mar 26, 2019
This defines a new 'isrpipe_read_timeout' module that should be used when using
the timeout based function of isrpipe.

This fix the implicit dependency to 'xtimer' that is only needed for the
'_timeout' functions.

It prevents 'stdio_uart' that uses 'isrpipe' to need to depend on xtimer.
This was silently solved at link time for most platforms but not for the
'esp32' for example.

'drivers/at' needed to be updated at the same time to follow the api change.
`at` is not using the `xtimer` module directly but only through
'isrpipe_read_timeout'.
@cladmi cladmi force-pushed the pr/isrpipe/separate_read_timeout branch from 875bfaa to 29fc58b Compare March 26, 2019 20:36
@cladmi
Copy link
Contributor Author

cladmi commented Mar 26, 2019

I indeed missed that the module is called "isrpipe" and the group "isr_pipe" which I applied to the new group name.
Fixed and squashed.

@miri64
Copy link
Member

miri64 commented Mar 26, 2019

I indeed missed that the module is called "isrpipe" and the group "isr_pipe" which I applied to the new group name.

we should fix the group naming actually, but not in this PR...

@miri64 miri64 merged commit db8954f into RIOT-OS:master Mar 26, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Mar 26, 2019

Thanks for the review.

@cladmi cladmi deleted the pr/isrpipe/separate_read_timeout branch September 10, 2019 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants