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

USB: add pure specifiers and emit vtable #771

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

ardnew
Copy link
Contributor

@ardnew ardnew commented Nov 23, 2023

Issue #130 correctly identifies a newly-added method as pure virtual and fixes the declaration. However, for some reason it didn't address all of the other virtual methods in that same class (PluggableUSBModule) that do not define a default implementation.

The only virtual method that has a default implementation is provided inline in the class interface:

    virtual void callback_reset() {};

These issues combined prevent the compiler from being able to emit a vtable for the PluggableUSBModule class, thus preventing users from correctly subclassing it or any one of its derived classes such as USBCDC, USBHID, USBMIDI, etc. Refer to the following answer on StackOverflow for a detailed explanation of the issue:

https://stackoverflow.com/a/57504289/1054397

This PR adds the pure specifier (= 0) to all of the virtual methods in this class that do not have a default implementation. It also moves the default empty definition of virtual void callback_reset() to the class definition in USB/PluggableUSBDevice.cpp so that this class complies completely with the criteria for emitting a vtable.

Note

The error that I was encountering prior to these changes was pretty
cryptic (from PlatformIO):

.pio/build/hid/src/target.cpp.o: In function `foo()':
USBHID/src/PluggableUSBHID.h:53: undefined reference to
    `vtable for arduino::internal::PluggableUSBModule'
.pio/build/hid/src/target.cpp.o: In function `foo()':
foo.hpp:100: undefined reference to
    `vtable for arduino::internal::PluggableUSBModule'
collect2: error: ld returned 1 exit status
*** [.pio/build/hid/firmware.elf] Error 1

Even stranger, the error would only be generated with a debug build;
i.e., the only difference in command-line arguments was the additional
CFLAGS of -Og -g2 -ggdb2. Without the debug flags, my project was
building without error.

With the changes in this PR, my project now builds with and without
these additional debug flags. Further verification was performed by
testing the example sketches Keyboard, KeyboardRaw, and Mouse
from library USBHID as well as using the core Serial object for
ordinary USB serial I/O (USBCDC).

Issue arduino#130 correctly identifies a newly-added method as pure virtual and
fixes the declaration. However, for some reason it didn't address all of
the other virtual methods in that same class (`PluggableUSBModule`) that
do not define a default implementation.

The only virtual method that has a default implementation is provided
inline in the class interface:

```c++
    virtual void callback_reset() {};
```

These issues combined prevent the compiler from being able to emit a
vtable for the `PluggableUSBModule` class, thus preventing users from
correctly subclassing it or any one of its derived classes such as
`USBCDC`, `USBHID`, `USBMIDI`, etc. Refer to the following answer on
StackOverflow for a detailed explanation of the issue:

https://stackoverflow.com/a/57504289/1054397

This PR adds the pure specifier (`= 0`) to all of the virtual methods in
this class that do not have a default implementation. It also moves the
default empty definition of `virtual void callback_reset()` to the class
definition in `USB/PluggableUSBDevice.cpp` so that this class complies
completely with the criteria for emitting a vtable.

> #### Note
>
> The error that I was encountering prior to these changes was pretty
> cryptic (from PlatformIO):
>
> ```txt
> .pio/build/hid/src/target.cpp.o: In function `foo()':
> USBHID/src/PluggableUSBHID.h:53: undefined reference to
>     `vtable for arduino::internal::PluggableUSBModule'
> .pio/build/hid/src/target.cpp.o: In function `foo()':
> foo.hpp:100: undefined reference to
>     `vtable for arduino::internal::PluggableUSBModule'
> collect2: error: ld returned 1 exit status
> *** [.pio/build/hid/firmware.elf] Error 1
> ```
>
> Even stranger, the error would only be generated with a debug build;
> i.e., the only difference in command-line arguments was the additional
> CFLAGS of `-Og -g2 -ggdb2`. Without the debug flags, my project was
> building without error.
>
> With the changes in this PR, my project now builds with and without
> these additional debug flags. Further verification was performed by
> testing the example sketches `Keyboard`, `KeyboardRaw`, and `Mouse`
> from library `USBHID` as well as using the core `Serial` object for
> ordinary USB serial I/O (`USBCDC`).
@CLAassistant
Copy link

CLAassistant commented Nov 23, 2023

CLA assistant check
All committers have signed the CLA.

@facchinm facchinm requested a review from alrvid November 23, 2023 08:29
@facchinm
Copy link
Member

facchinm commented Dec 4, 2023

LGTM! Thanks!

@facchinm facchinm merged commit fcbb950 into arduino:main Dec 4, 2023
11 checks passed
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.

3 participants