PluggableUSBModule::getProductVersion() not defined

[tttapa]

Commit 301d69d introduces a PluggableUSBModule::getProductVersion() method declaration, but it is not defined in the base class, resulting in a linker error when derived classes don't override it.

I believe this method should be pure virtual:

virtual uint8_t getProductVersion() = 0;

Is there a guideline for which value third-party libraries should use? Should they just set the next available bit (16)? Is there an upper bound?

From the name of the function alone, it was not really clear to me how the function is intended to be used. The links in the commit message explain the rationale behind it, but it might be useful to have a comment or docstring in the PluggableUSBModule class documenting how and why to use it when defining a module.

[facchinm]

@tttapa thanks for pointing that out! I didn't think there were any other module in development when I pushed it so I took the freedom to make a breaking change, my bad.
Feel free to post a PR for the pure virtual method so we keep the attribution; about the value to be assigned, it would be cool if it could be automaticlly generated in some way, but in the meantime using the next bit (16) is perfectly fine.

[tttapa]

@facchinm no problem, it's easy enough to fix :)

I've opened a pull request with the proposed change. When the method is not implemented in a derived PluggableUSBModule, you now get a clear compiler message instead of a linker error:

In file included from /home/pieter/Arduino/libraries/Control-Surface/src/MIDI_Interfaces/USBMIDI/USBMIDI.hpp:55:0,
                 from /home/pieter/Arduino/libraries/Control-Surface/examples/3. MIDI Interfaces/MIDI_Pipes-Routing/MIDI_Pipes-Routing.ino:14:
/home/pieter/Arduino/libraries/Control-Surface/src/MIDI_Interfaces/USBMIDI/USBMIDI_Arduino_mbed.hpp:19:22: error: cannot declare field 'CS::USBDeviceMIDIBackend::backend' to be of abstract type 'CS::PluggableUSBMIDI'
     PluggableUSBMIDI backend;
                      ^~~~~~~
In file included from /home/pieter/Arduino/libraries/Control-Surface/src/MIDI_Interfaces/USBMIDI/USBMIDI_Arduino_mbed.hpp:4:0,
                 from /home/pieter/Arduino/libraries/Control-Surface/examples/3. MIDI Interfaces/MIDI_Pipes-Routing/MIDI_Pipes-Routing.ino:14:
/home/pieter/Arduino/libraries/Control-Surface/src/MIDI_Interfaces/USBMIDI/mbed/PluggableUSBMIDI.hpp:18:7: note:   because the following virtual functions are pure within 'CS::PluggableUSBMIDI':
 class PluggableUSBMIDI : protected arduino::internal::PluggableUSBModule {
       ^~~~~~~~~~~~~~~~
In file included from /home/pieter/.arduino15/packages/arduino/hardware/mbed/1.3.2/cores/arduino/USB/USBCDC.h:27:0,
                 from /tmp/arduino_build_469037/sketch/MIDI_Pipes-Routing.ino.cpp:1:
/home/pieter/.arduino15/packages/arduino/hardware/mbed/1.3.2/cores/arduino/USB/PluggableUSBDevice.h:61:21: note: 	virtual uint8_t arduino::internal::PluggableUSBModule::getProductVersion()
     virtual uint8_t getProductVersion() = 0;
                     ^~~~~~~~~~~~~~~~~

instead of

/tmp/arduino_build_469037/libraries/Control-Surface/MIDI_Interfaces/USBMIDI/mbed/PluggableUSBMIDI.cpp.o:(.rodata._ZTVN2CS16PluggableUSBMIDIE+0x2c): undefined reference to `arduino::internal::PluggableUSBModule::getProductVersion()'
collect2: error: ld returned 1 exit status

It seems to be common in the ARM mbed os code, and you might already be aware of this, but one thing I noticed while trying to understand the PluggableUSB code is that none of the virtual methods are marked override. This makes it hard to know which virtual methods are overriding base class methods and which virtual methods are new methods that can be overridden by derived classes.

For example:

class USBCDC: public internal::PluggableUSBModule{
    virtual const uint8_t *string_iproduct_desc();
};

This implies to me that string_iproduct_desc is a new virtual method, introduced in the USBCDC class that derived classes from USBCDC may want to implement (which is not the case here).
On the other hand, the following implies that string_iproduct_desc is a virtual method of the internal::PluggableUSBModule class (or higher up the hierarchy) that USBCDC overrides:

class USBCDC: public internal::PluggableUSBModule{
    const uint8_t *string_iproduct_desc() override;
};

You can use the compiler to tell you which methods should be marked override using the -Wsuggest-override warning flag:

/home/pieter/.arduino15/packages/arduino/hardware/mbed/1.3.2/cores/arduino/USB/USBCDC.h:159:28: warning: 'virtual const uint8_t* arduino::USBCDC::string_iinterface_desc()' can be marked override [-Wsuggest-override]
     virtual const uint8_t *string_iinterface_desc();
                            ^~~~~~~~~~~~~~~~~~~~~~

[facchinm]

Closing as fixed by #148 .
Thanks for the override suggestion, feel free to post any additional PR to make it clearer 😉