fix some weak symbols in vendor device code#556
Closed
mucki-at wants to merge 5 commits intoadafruit:masterfrom
mucki-at:mucki-at/weak-fix
Closed
fix some weak symbols in vendor device code#556mucki-at wants to merge 5 commits intoadafruit:masterfrom mucki-at:mucki-at/weak-fix
mucki-at wants to merge 5 commits intoadafruit:masterfrom
mucki-at:mucki-at/weak-fix
Conversation
Author
|
Ok, tud_vendor_rx_cb were also touched in master which was hard to figure out because I cannot test your builds locally before submitting a PR, but it seems fixed now. The change is even smaller now :) |
Author
|
Spoke too soon. But I totally do not understand these new errors. Somehow the sample (which is not modified) conflicts with code in Arduino core which are not even part of this library? Can someone explain how you guys are building this, because I can't even see these files. |
Author
|
I misunderstood how the vendor interface in TinyUSB works. While I still think that the weak symbols are used wrong in this instance, I no longer need to interact with the functions changed in this PR, and since it does not compile properly I am abandoning it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The way TU_ATTR_WEAK (weak symbols for linking) in this library is problematic and inconsistent. Weak symbols have two usages:
To use option 1, the declaration (in the header) should be marked weak, so the compiler knows that the symbol is entirely optional. To use option 2, the declaration must not be marked weak, so the compiler knows that an implementation must be provided, but the library can provide a default implementation (marked weak in the implementation) so that the user can override.
Option 1 is only feasible if the number of implementations is either 0 or 1. Since all implementations are weak, the linker will pick an implementation at random during link time, so there is no control which version will end up in the final program if there are multiple.
Option 2 provides automatic safety against multiple strong definitions (a link error will result). The linker will always pick the strong implementation if it exists, otherwise a random weak one.
In the library some callbacks are using Option 1, and some are using Option 2. Even more unfortunate the tud_vendor_control_xfer_cb uses Option 1 and already provides a definition in the webusb code.
This patch does not fully address the issue, since this is a library design issue and requires choices only the maintainer can make, however it fixes:
my recommendation would be to either remove all weak annotations from headers and only use them in the implementations (tinyusb style), or expose completely new symbols which do not conflict with tinyusb names and avoid problems like only one vendor class callback when actually multiple might be needed.