Skip to content

isrpipe: split isrpipe_read_timeout to isolate xtimer dependency - #11267

Merged
miri64 merged 2 commits into
RIOT-OS:masterfrom
cladmi:pr/isrpipe/separate_read_timeout
Mar 26, 2019
Merged

isrpipe: split isrpipe_read_timeout to isolate xtimer dependency #11267
miri64 merged 2 commits into
RIOT-OS:masterfrom
cladmi:pr/isrpipe/separate_read_timeout

Conversation

@cladmi

@cladmi cladmi commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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 kaspar030 and miri64 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

cladmi commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

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

cladmi commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

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

@miri64

miri64 commented Mar 26, 2019

Copy link
Copy Markdown
Member

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

Please feel free to do so.

@miri64 miri64 left a comment

Copy link
Copy Markdown
Member

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.

Comment thread sys/include/isrpipe/read_timeout.h Outdated
*/

/**
* @defgroup isr_pipe ISR Pipe

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

cladmi commented Mar 26, 2019

Copy link
Copy Markdown
Contributor Author

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

@miri64 miri64 left a comment

Copy link
Copy Markdown
Member

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

miri64 commented Mar 26, 2019

Copy link
Copy Markdown
Member

Please squash

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

cladmi commented Mar 26, 2019

Copy link
Copy Markdown
Contributor Author

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

@miri64 miri64 left a comment

Copy link
Copy Markdown
Member

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
cladmi added 2 commits March 26, 2019 21:36
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

cladmi commented Mar 26, 2019

Copy link
Copy Markdown
Contributor Author

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

miri64 commented Mar 26, 2019

Copy link
Copy Markdown
Member

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

cladmi commented Mar 26, 2019

Copy link
Copy Markdown
Contributor Author

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.

2 participants