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.

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

stm32/usb: Add usb mode for VCP+VCP without MSC #4712

Closed

Conversation

andrewleech
Copy link
Sponsor Contributor

When MICROPY_HW_USB_ENABLE_CDC2 is enabled, this allows a usb mode of VCP+VCP rather than just the existing VCP+VCP+MSC for situation where two serial ports are desired, but mass storage is not.

I found it rather difficult to modify an existing configuration similar to the way the current CDC2 config was added onto VCP+MSC at runtime, so this includes a complete new template struct.

@hoihu
Copy link
Sponsor Contributor

hoihu commented Apr 23, 2019

+1 for the need of a dual cdc mode without msc

@dpgeorge
Copy link
Member

Thanks for this, it looks pretty good!

I found it rather difficult to modify an existing configuration similar to the way the current CDC2 config was added onto VCP+MSC at runtime, so this includes a complete new template struct.

I think I'll have a go at optimising the existing code so it doesn't duplicate these templates everywhere, then hopefully the PR here can be a lot simpler.

@dpgeorge
Copy link
Member

I think I'll have a go at optimising the existing code so it doesn't duplicate these templates everywhere, then hopefully the PR here can be a lot simpler.

See #4721

@andrewleech
Copy link
Sponsor Contributor Author

Ok, as a bonus/bloat I've tacked this on here as well for now: f455ccf

It automatically tracks the appropriate interface number for each definition, making it even easier to add any new mix of descriptors. It does add a little more code size again though, so up to you if you think it adds enough value.

@dpgeorge
Copy link
Member

It automatically tracks the appropriate interface number for each definition, making it even easier to add any new mix of descriptors

I did consider this but didn't go as far as implementing it. I didn't want to have too much code increase. But maybe it's worth it to simplify the configuration.

@@ -311,6 +311,11 @@ STATIC mp_obj_t pyb_usb_mode(size_t n_args, const mp_obj_t *pos_args, mp_map_t *
pid = USBD_PID_CDC2_MSC;
}
mode = USBD_MODE_CDC2_MSC;
} else if (strcmp(mode_str, "CDC+CDC") == 0 || strcmp(mode_str, "VCP+VCP") == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

CDC is legacy so we don't need to add it here, VCP is enough.

@@ -462,45 +458,54 @@ int USBD_SelectMode(usbd_cdc_msc_hid_state_t *usbd, uint32_t mode, USBD_HID_Mode
uint8_t num_itf = 0;
switch (usbd->usbd_mode) {
case USBD_MODE_MSC:
n += make_msc_desc(d + n);
num_itf = 1;
usbd->MSC_BOT_ClassData.interface = num_itf;
Copy link
Member

Choose a reason for hiding this comment

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

Can you really use this variable MSC_BOT_ClassData.interface safely? Note that the USB_REQ_SET_INTERFACE command will change this value... not sure what to but it might be better to use a separate (new) variable for this.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Tbh this hasn't been thoroughly tested, last build I didn't have msc enabled.

@andrewleech
Copy link
Sponsor Contributor Author

Thinking about it more, I'm not sure the interface counting is really with it yet. It doesn't actually simplify things much, certainly not compared to your change which was a dramatic improvement for the code size cost.

I would hold off on my second commit, perhaps it can be the start of a bigger dynamic improvement later

@dpgeorge
Copy link
Member

Ok I'll just go for the VCP+VCP commit.

@dpgeorge
Copy link
Member

The VCP+VCP commit/feature was merged in 70a28e3

@dpgeorge dpgeorge closed this Apr 28, 2019
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants