Skip to content

k_work conversion for network related components #34087

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 14, 2021
Merged

k_work conversion for network related components #34087

merged 6 commits into from
Apr 14, 2021

Conversation

jukkar
Copy link
Member

@jukkar jukkar commented Apr 7, 2021

This is related to #33104 and takes relevant patches from #33924. I did slight changes to some of the commits from #33824, those ones have my signed-off-by added.
Only minimal testing done for those components that I could, but overall the original rote conversion by @pabigot looked good.

@jukkar jukkar requested review from pabigot and rlubos April 7, 2021 14:27
@jukkar jukkar mentioned this pull request Apr 7, 2021
Closed
18 tasks
@jukkar jukkar force-pushed the k_work-conversion branch from ceb854d to eb44cd6 Compare April 8, 2021 07:13
pabigot
pabigot reviewed Apr 9, 2021
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I can't approve this officially since the commits are mostly from me, but I see no problem with these changes.

rlubos
rlubos reviewed Apr 9, 2021
View reviewed changes
Copy link
Collaborator

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

The API change looks reasonable to me, there are some style issues worth fixing I guess.

drivers/ieee802154/ieee802154_dw1000.c Outdated
@@ -1607,10 +1607,10 @@ static int dw1000_init(const struct device *dev)
DWT_SYS_MASK_MRXSFDTO);

/* Initialize IRQ event work queue */
k_work_q_start(&dwt_work_queue,
k_work_queue_start(&dwt_work_queue,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really related to the rework, but his work queue seems a bit suspicious, it doesn't actually seem to be used anywhere in the driver. CC @jfischer-no

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think k_work_submit should actually be k_work_submit_to_queue(&dwt_work_queue, &ctx->irq_cb_work); I will look through my WIP branches.

drivers/ethernet/eth_sam_gmac.c Outdated
k_delayed_work_init(&dev_data->monitor_work, monitor_work_handler);
k_delayed_work_submit(&dev_data->monitor_work,
k_work_init_delayable(&dev_data->monitor_work, monitor_work_handler);
k_work_reschedule(&dev_data->monitor_work,
K_MSEC(CONFIG_ETH_SAM_GMAC_MONITOR_PERIOD));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: there are quite a few places in this PR with broken alignment after the API change, like here, probably worth pushing the changes through some formatter.

Copy link
Member Author

Choose a reason for hiding this comment

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

there are quite a few places in this PR with broken alignment after the API change, like here, probably worth pushing the changes through some formatter.

Yeah, there are now these style issues in the code. I went through the changes in the subsys/net/* and manually changed those. I did not check all the changes in this PR (especially in the drivers/) as there was just so much of it. Using formatter could work except that it can change lines that we not changing in this k_work conversion, so I did not use a formatter. I think this work can be done in a separate PR if really needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using formatter could work except that it can change lines that we not changing in this k_work conversion, so I did not use a formatter.

I typically use git-clang-format HEAD~1 to apply the formatting only to the changes from the most recent commit, this combines well with an interactive rebase. But I now noticed that while it fixes the alignment issues I spotted it also introduce some others (like for instance changing the commit alignment from tabs to spaces which in effect doesn't look good with other lines aligned with tabs). As it's really not that important I guess it could indeed be fixed at some point in the future in a more comprehensive approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

use git-clang-format HEAD~1 to apply the formatting only to the changes from the most recent commit

Thanks for the hint, I was not aware of this tool.

Copy link
Member Author

Choose a reason for hiding this comment

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

As it's really not that important I guess it could indeed be fixed at some point in the future in a more comprehensive approach.

I will revisit this PR and update the code. I did not realize there was a tool for doing the work and it looked a lot of work to go through the patches manually.

@jukkar
Copy link
Member Author

jukkar commented Apr 12, 2021

I can't approve this officially since the commits are mostly from me, but I see no problem with these changes.

Sure you can as you are not the sender of this PR. This one needs approvers and nobody seems to willing to accept the changes.

@pabigot pabigot requested review from galak and carlescufi April 12, 2021 16:33
pabigot added 6 commits April 13, 2021 13:25
@pabigot @jukkar
drivers: ethernet: rote conversion of k_work API
852efc0 
Replace all existing deprecated API with the recommended alternative.

Be aware that this does not address architectural errors in the use
of the work API.

Signed-off-by: Peter Bigot <[email protected]>
@pabigot @jukkar
drivers: ieee802154: rote conversion of k_work API
c4c9d2b 
Replace all existing deprecated API with the recommended alternative.

Be aware that this does not address architectural errors in the use
of the work API.

Signed-off-by: Peter Bigot <[email protected]>
@pabigot @jukkar
drivers: wifi: rote conversion of k_work API
e9ceed0 
Replace all existing deprecated API with the recommended alternative.

Be aware that this does not address architectural errors in the use
of the work API.

Signed-off-by: Peter Bigot <[email protected]>
@pabigot @jukkar
drivers: modem: rote conversion of k_work API
5a2a392 
Replace all existing deprecated API with the recommended alternative.

Be aware that this does not address architectural errors in the use
of the work API.

Signed-off-by: Peter Bigot <[email protected]>
Signed-off-by: Jukka Rissanen <[email protected]>
@pabigot @jukkar
drivers: console: mux: Conversion of k_work API
8e50e6b 
Replace all existing deprecated API with the recommended alternative
for gsm_mux and uart_mux drivers.

Fixes zephyrproject-rtos#34103

Signed-off-by: Peter Bigot <[email protected]>
Signed-off-by: Jukka Rissanen <[email protected]>
@pabigot @jukkar
net: Conversion of k_work API
091bddd 
Replace all existing deprecated API with the recommended alternative.

Signed-off-by: Peter Bigot <[email protected]>
Signed-off-by: Jukka Rissanen <[email protected]>
@jukkar jukkar force-pushed the k_work-conversion branch from eb44cd6 to 091bddd Compare April 13, 2021 10:30
@jukkar
Copy link
Member Author

jukkar commented Apr 13, 2021

Fixed the code indentation

@pabigot
Copy link
Collaborator

pabigot commented Apr 13, 2021

@rlubos @jukkar If people are interested in using clang-format somebody could take over #27058. I think the list of FOR-EACH macros probably needs to be updated.

@nashif nashif merged commit 188cb2c into zephyrproject-rtos:master Apr 14, 2021
@jukkar jukkar deleted the k_work-conversion branch April 26, 2021 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pabigot pabigot pabigot left review comments

@jfischer-no jfischer-no jfischer-no approved these changes

@rlubos rlubos rlubos approved these changes

@alexanderwachter alexanderwachter Awaiting requested review from alexanderwachter

@loicpoulain loicpoulain Awaiting requested review from loicpoulain

@mniestroj mniestroj Awaiting requested review from mniestroj

@nandojve nandojve Awaiting requested review from nandojve

@pfalcon pfalcon Awaiting requested review from pfalcon

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

@karstenkoenig karstenkoenig Awaiting requested review from karstenkoenig

@Kludentwo Kludentwo Awaiting requested review from Kludentwo

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@mengxianglinx mengxianglinx Awaiting requested review from mengxianglinx

@galak galak Awaiting requested review from galak

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees

@jukkar jukkar

Labels
area: API Changes to public APIs area: CAN area: Console area: Ethernet area: IEEE 802.15.4 area: LWM2M area: Modem area: Networking area: Samples Samples area: Sockets Networking sockets area: Wi-Fi Wi-Fi platform: NXP NXP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jukkar @pabigot @jfischer-no @rlubos @nashif @zephyrbot