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

Kconfig: Expose USB configurations #12936

Merged
merged 8 commits into from
Jan 14, 2020
Merged

Kconfig: Expose USB configurations #12936

merged 8 commits into from
Jan 14, 2020

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Dec 12, 2019 •
edited
Loading

Contribution description

This PR moves configuration macros of USB to the CONFIG_ name space and exposes them to Kconfig.

I also added a Kconfig to examples/usb_minimal to have VID/PID defaults only for that application. Currently the PR depends on #12913 (97065a8), because I added a check in the application's Makefile to not set the VID/PID via CFLAGS when Kconfig is already being used for that.

Testing procedure

  • By default examples/usb_minimal should use all the default configurations as always.
  • Running make menuconfig should allow to configure USB. When ran in examples/usb_minimal VID/PID should have default values. If the default values are changed the warning message should not appear.

Issues/PRs references

Depends on #12913
Part of #12888

@leandrolanzieri leandrolanzieri added Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first TF: Config Marks issues and PRs related to the work of the Configuration Task Force Area: USB Area: Universal Serial Bus labels Dec 12, 2019
@leandrolanzieri leandrolanzieri added this to the Release 2020.01 milestone Dec 12, 2019
@leandrolanzieri leandrolanzieri mentioned this pull request Dec 12, 2019
Open
71 tasks
@leandrolanzieri
Copy link
Contributor Author

Rebased to master, this has no dependencies now.

@leandrolanzieri leandrolanzieri removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 19, 2019
bergzand
bergzand previously requested changes Dec 19, 2019
View reviewed changes
sys/usb/Kconfig Outdated Show resolved Hide resolved
sys/usb/Kconfig Outdated Show resolved Hide resolved
sys/usb/Kconfig Outdated Show resolved Hide resolved
sys/usb/Kconfig Outdated Show resolved Hide resolved
@bergzand
Copy link
Member

I'm not confident in reviewing the modifications to the minimal usbus example, maybe somebody else could look into that

bergzand
bergzand reviewed Dec 19, 2019
View reviewed changes
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Some option naming nitpicks.

Is there a consensus (or a rough idea) on a option naming scheme?

sys/usb/Kconfig Show resolved Hide resolved
sys/usb/Kconfig Outdated Show resolved Hide resolved
sys/usb/Kconfig Outdated Show resolved Hide resolved
sys/usb/Kconfig Outdated Show resolved Hide resolved
@leandrolanzieri
Copy link
Contributor Author

I'm not confident in reviewing the modifications to the minimal usbus example, maybe somebody else could look into that

Maybe @cgundogan can take a look?

@cgundogan
Copy link
Member

Maybe @cgundogan can take a look?

the changes to the usbus example app are small .. IMO they look fine. I will run a test today and return with my results.

@cgundogan
Copy link
Member

the changes to the usbus example app are small .. IMO they look
fine. I will run a test today and return with my results.

I did a small test using a samr21 board and changed the configurations with menuconfig. My system showed the new configurations once I plugged in the user USB into my laptop.

@fjmolinas
Copy link
Contributor

@cgundogan @bergzand is this ok to squash?

@tcschmidt
Copy link
Member

Ping @bergzand @cgundogan

@bergzand
Copy link
Member

I'm fine with the changes here. I don't have the time at the moment to put this through a test, I have to leave that to the next reviewer :(

@bergzand bergzand added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jan 14, 2020
@bergzand bergzand dismissed their stale review January 14, 2020 09:14

Change requests are resolved

@bergzand bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 14, 2020
@cgundogan cgundogan added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 14, 2020
cgundogan
cgundogan approved these changes Jan 14, 2020
View reviewed changes
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

Also OK to squash from my side!

@leandrolanzieri
usb: Move configuration macros to 'CONFIG_' namespace
15bd489 
Macros that changed:
USB_CONFIG_VID -> CONFIG_USB_VID
USB_CONFIG_PID -> CONFIG_USB_PID
USB_CONFIG_MANUF_STR -> CONFIG_USB_MANUF_STR
USB_CONFIG_PRODUCT_STR -> CONFIG_USB_PRODUCT_STR
USB_CONFIG_CONFIGURATION_STR -> CONFIG_USB_CONFIGURATION_STR
USB_CONFIG_PRODUCT_BCDVERSION -> CONFIG_USB_PRODUCT_BCDVERSION
USB_CONFIG_SPEC_BCDVERSION -> CONFIG_USB_SPEC_BCDVERSION
USB_CONFIG_SELF_POWERED -> CONFIG_USB_SELF_POWERED
USB_CONFIG_MAX_POWER -> CONFIG_USB_MAX_POWER
USB_CONFIG_DEFAULT_LANGID -> CONFIG_USB_DEFAULT_LANGID
@leandrolanzieri
Kconfig: Expose USB peripheral configurations
a1836d3
@leandrolanzieri
examples/usb_minimal: Set PID/VID if Kconfig is not used
84c3676
@leandrolanzieri
examples/usbus_minimal: Add default Kconfig PID/VID values
903909f
@leandrolanzieri
Copy link
Contributor Author

Also OK to squash from my side!

@cgundogan squashed

@miri64 miri64 merged commit 350b33b into RIOT-OS:master Jan 14, 2020
@leandrolanzieri leandrolanzieri deleted the pr/kconfig_migrate/usb branch January 14, 2020 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@bergzand bergzand bergzand left review comments

@cgundogan cgundogan cgundogan approved these changes

Assignees
No one assigned
Labels
Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines TF: Config Marks issues and PRs related to the work of the Configuration Task Force Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.
6 participants
@leandrolanzieri @bergzand @cgundogan @fjmolinas @tcschmidt @miri64