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

tinyusb: Allow to set the USB manufacturer/product identifiers #172

Merged
merged 1 commit into from
Sep 10, 2019
Merged

tinyusb: Allow to set the USB manufacturer/product identifiers #172

merged 1 commit into from
Sep 10, 2019

Conversation

kaysievers
Copy link

This allows the sketch to override the USB indentifier strings. It is useful
if multiple devices are connected to the same host. Or MIDI devices show up
by thier given name in the software which uses them. Before this change
it was only possible by writing a custom board config.

Tinyusb made it easy to just add a simple separate C file to a sketch, which
will provide the custom names. They are picked up by the linker and replace
the default names provided by the board config.

Example file: usb-names.c
const char *USBDeviceManufacturer = "MyManufacturer";
const char *USBDeviceProduct = "MyProduct";

Tested with an 'Adafruit ItsyBitsy M0 Express' and tinyusb (the classic USB
stack is not supported).

@ladyada ladyada requested a review from hathach August 26, 2019 18:52
Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

prefer to have explicit API

@@ -38,6 +38,18 @@
#define USB_CONFIG_POWER 100
#endif

// Weak symbols which can be replaced at link time to provide custom
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your PR. This is a good approach, but I would prefer a more explicit way to do so with an API, much like the Adafruit_USBD_Device::setID() . Let's add more API for this purpose

- setManufacturerDescriptor(uint16_t const* desc)
- setProductDescriptor(uint16_t const* desc)
- setLanguageID(uint16_t language_id)

default values will be used if these are not set. Note: input value is UTF16 since user may want to use their local language.

If you don't have time, I will eventually implement these when I got my time to work with usb again.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, when would that API run? Aren't the names set and passed to USB in the device constructor at init time, before user code could change it?

The PR is linker logic, the original strings are kicked out by the build process.

Currently the strings are ASCII and converted internally on-the-fly when called. You want to change that?

Copy link
Member

Choose a reason for hiding this comment

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

Those can be called in the setup(), those aren't set anywhere, tud_descriptor_string_cb() is a callback invoked by tinyUSB stack when receiving GET_DESCRIPTOR request from host. We can literally change how it response anytime (as long as it happens before host ask for it).

The current string is ascii and converted on-the-fly since I know for sure they are all English. If we allow custom descriptor, we should make it possible for user to use their own language.

If you don't have time or not comfortable to do this, don't worry, I will do it later on :)

Copy link
Author

@kaysievers kaysievers Aug 27, 2019

Choose a reason for hiding this comment

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

  • setManufacturerDescriptor(uint16_t const* desc)
  • setProductDescriptor(uint16_t const* desc)
  • setLanguageID(uint16_t language_id)

The USB localization is meant to carry multiple languages at the same time in the same descriptor. Do you want to store one language id globally in the device for all strings?

Those can be called in the setup(), ... We can literally change how it response anytime (as long as it happens before host ask for it).

Ah, so we could allow changing it during runtime, just check that the device is not mounted. That sounds nice.

Copy link
Author

Choose a reason for hiding this comment

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

Here is a simple version that seem to work nicely:
https://github.com/kaysievers/ArduinoCore-samd/tree/tinyusb-set-names

It might be really useful to be able to change the name during runtime, as long as the device is not in use by a host.

Copy link
Author

Choose a reason for hiding this comment

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

Do you want code to convert utf8 to utf16 properly?

If there isn't anything available, I could copy things from here. I wrote that code many many years ago. :)

https://git.kernel.org/pub/scm/linux/hotplug/udev.git/tree/src/libudev-util.c#n250

Copy link
Member

@hathach hathach Aug 27, 2019

Choose a reason for hiding this comment

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

The USB localization is meant to carry multiple languages at the same time in the same descriptor. Do you want to store one language id globally in the device for all strings?

For now, we don't need to support multiple languages, one language is good enough. if user use their local language for manufacturer/product e.g French, he/she should set the language ID accordingly to French as well.

Ah, so we could allow changing it during runtime, just check that the device is not mounted. That sounds nice.

There is no need to check as well, just set it. It will take affect the next time device is enumerated, user may force an dettach() then attach() for that ( note dettach()/attach() is not yet implemented yet, but soon).

Do you want code to convert utf8 to utf16 properly?

If there isn't anything available, I could copy things from here. I wrote that code many many years ago. :)

https://git.kernel.org/pub/scm/linux/hotplug/udev.git/tree/src/libudev-util.c#n250

No, the stack doesn't need to convert that, user sketch should give the input as the UTF16 string , which is the native format of string descriptor.

setManufacturerDescriptor(uint16_t const* desc)

Sorry if I didn't make it clear enough for you. Though, we can have 2 variants one with char* for ascii as you did, the other is uint16_t* for UTF16. Please update this PR, so that I could review and recommend what should be changed for the merge :) . I will add the utf16 variant later

Copy link
Author

@kaysievers kaysievers Aug 27, 2019

Choose a reason for hiding this comment

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

Updated. It accepts plain UTF-8 now and converts it to simple UTF-16. This way it also accepts internationalized names from the core config (without using this API), which are 8 bit chars only.

See the screenshot :)

Screen Shot 2019-08-27 at 11 50 56

Set the USB descriptor strings. I accepts UTF-8 strings with
codepoints up to 16 bit.

  void setup() {
    USBDevice.setManufacturer("MyManufacturer");
    USBDevice.setProduct("MyProduct");
  }
@kaysievers kaysievers changed the title usb: Add a simple way to override the USB manufacturer/product strings tinyusb: Allow to set the USB manufacturer/product identifiers Aug 27, 2019
@hathach hathach self-assigned this Sep 3, 2019
Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

sorry for late response, I have been busy with other mcu port for the tinyusb stack and fixing other urgent bug. Superb !!! Thank you very much for your work and patient.

@hathach hathach merged commit c4f3490 into adafruit:master Sep 10, 2019
@hathach
Copy link
Member

hathach commented Sep 10, 2019

PS: I will add setLanguageDecriptor since it need to be updated once we use other language than English. Also add the Descriptor suffix to API --> setManufacturerDescriptor()/setProductDescriptor() just to make it clearer.

@hathach hathach mentioned this pull request Sep 10, 2019
hathach added a commit that referenced this pull request Sep 10, 2019
@kaysievers kaysievers deleted the usb-names-override branch September 10, 2019 15:39
@deladriere
Copy link

Can we use this with the Adafruit_USBD_MIDI ?

@kaysievers
Copy link
Author

Can we use this with the Adafruit_USBD_MIDI ?

Yes, the screenshot is a MIDI device using tinyusb MIDI:
#172 (comment)

@deladriere
Copy link

deladriere commented Sep 30, 2019

@kaysievers
I am compiling the midi test example
https://github.com/adafruit/Adafruit_TinyUSB_Arduino/tree/master/examples/MIDI/midi_test
but adding these line in the setup

MIDI.setManufacturer("MyManufacturer"); MIDI.setProduct("MyProduct");
gives an error
midi_test:49:8: error: 'class midi::MidiInterface<Adafruit_USBD_MIDI>' has no member named 'setManufacturer' MIDI.setManufacturer("MyManufacturer"); ^ midi_test:50:10: error: 'class midi::MidiInterface<Adafruit_USBD_MIDI>' has no member named 'setProduct' MIDI.setProduct("MyProduct");

@kaysievers
Copy link
Author

A later change renamed the method names to setManufacturerDescriptor(), setProductDescriptor()

7c02454

@deladriere
Copy link

deladriere commented Oct 1, 2019

Ok I have downloaded the master tree and use it to replace the 1.5.3 in the Arduino IDE
now I have the following error
Library/Arduino15/packages/adafruit/hardware/samd/1.5.3/cores/arduino/wiring.c: In function 'init': Library/Arduino15/packages/adafruit/hardware/samd/1.5.3/cores/arduino/wiring.c:95:132: error: lvalue required as left operand of assignment PM->APBCMASK.reg |= PM_APBCMASK_TCC0 | PM_APBCMASK_TCC1 | PM_APBCMASK_TCC2 | PM_APBCMASK_TC3 | PM_APBCMASK_TC4 | PM_APBCMASK_TC5 |= PM_APBCMASK_TC6 | PM_APBCMASK_TC7;
Thanks in advance

@deladriere
Copy link

I edited the file wiring.c to remove the = from the line and it works now!

@kaysievers
Copy link
Author

kaysievers commented Oct 1, 2019

I edited the file wiring.c to remove the = from the line

I reported it here in change which introduced it:
#174 (comment)

Thanks!

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

3 participants