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.

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

sys/net/netopt: Drop deprecated NETOPT_MAX_PACKET_SIZE #16023

Merged
merged 9 commits into from
Aug 26, 2021
Merged

sys/net/netopt: Drop deprecated NETOPT_MAX_PACKET_SIZE #16023

merged 9 commits into from
Aug 26, 2021

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 16, 2021

Contribution description

Drop the deprecated alias NETOPT_MAX_PACKET_SIZE for NETOPT_MAX_PDU_SIZE.

Testing procedure

Murdock will do here.

Issues/PRs references

None

@maribu
tests/gnrc_sixlowpan_frag*: Use NETOPT_MAX_PDU_SIZE
bcca86b 
Replaces instances of deprecated NETOPT_MAX_PACKET_SIZE by NETOPT_MAX_PDU_SIZE.
@maribu maribu added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: sys Area: System labels Feb 16, 2021
@maribu maribu requested a review from miri64 February 16, 2021 13:12
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 16, 2021
@kaspar030 kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 16, 2021
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 24, 2021
miri64
miri64 approved these changes Feb 25, 2021
View reviewed changes
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.

Though no removal release was deprecated over a year ago

$ git blame sys/include/net/netopt.h
[…]
5ba72df5aae (Marian Buschsieweke 2019-02-04 12:38:25 +0100  32) /**
5ba72df5aae (Marian Buschsieweke 2019-02-04 12:38:25 +0100  33)  * @brief       A deprecated alias for @ref NETOPT_MAX_PDU_SIZE
5ba72df5aae (Marian Buschsieweke 2019-02-04 12:38:25 +0100  34)  *
5ba72df5aae (Marian Buschsieweke 2019-02-04 12:38:25 +0100  35)  * @deprecated  Please use @ref NETOPT_MAX_PDU_SIZE instead of
5ba72df5aae (Marian Buschsieweke 2019-02-04 12:38:25 +0100  36)  *              `NETOPT_MAX_PACKET_SIZE`
5ba72df5aae (Marian Buschsieweke 2019-02-04 12:38:25 +0100  37)  */
5ba72df5aae (Marian Buschsieweke 2019-02-04 12:38:25 +0100  38) #define NETOPT_MAX_PACKET_SIZE NETOPT_MAX_PDU_SIZE
[…]

So IMHO this is okay to remove.

@miri64
Copy link
Member

miri64 commented Feb 25, 2021

ccn-lite and ndn-riot still use that netopt though :-/

@maribu
Copy link
Member Author

maribu commented Feb 25, 2021 •
edited
Loading

See: cn-uofbasel/ccn-lite#379 and named-data-iot/ndn-riot#16

@miri64
Copy link
Member

miri64 commented Feb 25, 2021

See: cn-uofbasel/ccn-lite#379 and named-data-iot/ndn-riot#16

The first I merged already, for the second I am not sure if it makes sense to go with a patch for now, since I do not know, how active the project still is.

@miri64
Copy link
Member

miri64 commented Feb 25, 2021

Unrelated, I know, but do you maybe want to piggy-back some style fixes for the issues pointed out by the static tests? I wouldn't ask if I did not know you are usually open to this :-).

miri64
miri64 approved these changes Feb 25, 2021
View reviewed changes
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.

Re-ACK from my side. Though I would prefer a reordering of the commits such that the removal of the usage in the packets comes first and the removal of the option comes after. This way, the history in this PR stays bisectable without compile errors.

@miri64
Copy link
Member

miri64 commented Feb 25, 2021

(might be that this is already the case and Github just shows it sorted by author date; did not check)

@maribu
pkg/ndn-riot: use NETOPT_MAX_PDU_SIZE
0da0f3a 
Replace NETOPT_MAX_PACKET_SIZE by NETOPT_MAX_PDU_SIZE via a patch
@maribu
pkg/ccn-lite: bump version
0d2c73d
@maribu
sys/net/netopt: Drop deprecated NETOPT_MAX_PACKET_SIZE
38a94fb 
This option has been deprecated for quite a few release. It should be safe to
delete now.
@maribu
sys/net/netopt: minor style fixes
0c349cd
@maribu
Copy link
Member Author

maribu commented Feb 25, 2021

I reordered the commits as suggested

@maribu
Copy link
Member Author

maribu commented Mar 2, 2021 

/tmp/dwq.0.10458823040246812/0caebe40f48da371dc7c0ff2fe626779/cpu/stm32/include/vendor/cmsis/f1/Include/stm32f1xx.h:167:3: note: in expansion of macro 'ERROR'
  167 |   ERROR = !SUCCESS

Cool! :-( We really should try to clean up RIOT so that vendor files are only included in cpu/* and not in any public header.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@github-actions github-actions bot added the Area: build system Area: Build system label Aug 25, 2021
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: network Area: Networking Area: pkg Area: External package ports Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Aug 25, 2021
@benpicco
Copy link
Contributor

What if we just patch the offending vendor files?
The ErrorStatus enum is never used by us.

@miri64
Copy link
Member

miri64 commented Aug 25, 2021 

/tmp/dwq.0.10458823040246812/0caebe40f48da371dc7c0ff2fe626779/cpu/stm32/include/vendor/cmsis/f1/Include/stm32f1xx.h:167:3: note: in expansion of macro 'ERROR'
  167 |   ERROR = !SUCCESS

I don't quiet understand, how this error was triggered in the first place...

@benpicco benpicco force-pushed the max_pdu_size branch 2 times, most recently from fdc150c to 7635f5f Compare August 25, 2021 14:39
@miri64
Copy link
Member

miri64 commented Aug 25, 2021 

/tmp/dwq.0.10458823040246812/0caebe40f48da371dc7c0ff2fe626779/cpu/stm32/include/vendor/cmsis/f1/Include/stm32f1xx.h:167:3: note: in expansion of macro 'ERROR'
  167 |   ERROR = !SUCCESS

I don't quiet understand, how this error was triggered in the first place...

Settled offline.

@chrysn chrysn added 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 labels Aug 25, 2021
@chrysn chrysn mentioned this pull request Aug 25, 2021
Merged
@benpicco benpicco merged commit 0b69747 into RIOT-OS:master Aug 26, 2021
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
@maribu maribu deleted the max_pdu_size branch January 23, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@miri64 miri64 miri64 approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2021.10
Development

Successfully merging this pull request may close these issues.
6 participants
@maribu @miri64 @benpicco @chrysn @kaspar030 @MrKevinWeiss