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

esp-wifi: Remove unneeded features #2446

Merged
merged 10 commits into from
Nov 8, 2024
Merged

esp-wifi: Remove unneeded features #2446

merged 10 commits into from
Nov 8, 2024

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Nov 1, 2024 •
edited
Loading

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Removes or moves features to configuration options where possible. The majority of the remaining features are associated with the wifi stack, which will be addressed in #stack

Closes #2305

MabezDev
MabezDev commented Nov 1, 2024
View reviewed changes
esp-config/src/generate.rs Outdated Show resolved Hide resolved
@MabezDev MabezDev force-pushed the esp-wifi-remove-features branch from 11d66f6 to 6af0c4a Compare November 5, 2024 16:07
@MabezDev MabezDev force-pushed the esp-wifi-remove-features branch from f722919 to 62fc1c6 Compare November 5, 2024 16:38
bugadani
bugadani reviewed Nov 6, 2024
View reviewed changes
esp-wifi/src/wifi/mod.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 7, 2024

Might be worth to mention phy-enable-usb is now enabled by default

@MabezDev MabezDev force-pushed the esp-wifi-remove-features branch from 1338c31 to 9a3811e Compare November 7, 2024 22:34
@jessebraham
Copy link
Member

Initial scan of the changes seem reasonable, will try to do a more thorough review today. Would you be able to get CI green in the meantime, please?

@MabezDev MabezDev force-pushed the esp-wifi-remove-features branch from 9a3811e to 449f7ba Compare November 8, 2024 10:37
@MabezDev MabezDev force-pushed the esp-wifi-remove-features branch from 449f7ba to c23613a Compare November 8, 2024 11:04
jessebraham
jessebraham approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 8, 2024

Seems we lost the ability to set ps-mode for esp-now only operation but on the other hand I'm not sure if that is even something we should offer (see https://www.esp32.com/viewtopic.php?t=19128)

bjoernQ
bjoernQ approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

JurajSadel
JurajSadel approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jessebraham jessebraham added this pull request to the merge queue Nov 8, 2024
@MabezDev
Copy link
Member Author

MabezDev commented Nov 8, 2024

Seems we lost the ability to set ps-mode for esp-now only operation but on the other hand I'm not sure if that is even something we should offer (see esp32.com/viewtopic.php?t=19128)

Did we? It's available on EspNowManager or am I misunderstanding?

Merged via the queue into esp-rs:main with commit 50d8187 Nov 8, 2024
28 checks passed
@MabezDev MabezDev deleted the esp-wifi-remove-features branch November 8, 2024 13:45
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 8, 2024

Seems we lost the ability to set ps-mode for esp-now only operation but on the other hand I'm not sure if that is even something we should offer (see esp32.com/viewtopic.php?t=19128)

Did we? It's available on EspNowManager or am I misunderstanding?

Oh it is .... somehow missed that from the diff 👍

bugadani
bugadani reviewed Nov 18, 2024
View reviewed changes
esp-wifi/CHANGELOG.md
Comment on lines +32 to +33
- Features `phy-enable-usb` & `dump-packets` have been turned into configuration options `phy_enable_usb` & `dump_packets` (#2446)
- Features `ps-min-modem` & `ps-max-modem` have been removed in favour of a runtime config (#2446)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to see these in the migration guide

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bugadani bugadani bugadani left review comments

@JurajSadel JurajSadel JurajSadel approved these changes

@bjoernQ bjoernQ bjoernQ approved these changes

@jessebraham jessebraham jessebraham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove esp-wifi features where possible
5 participants
@MabezDev @bjoernQ @jessebraham @bugadani @JurajSadel