search

twitchyliquid64 / usbd-hid

MIT License
88 stars 37 forks source link

Incompatibility with syn 1.0.76 ? #32

Closed jyelloz closed 3 years ago

jyelloz commented 3 years ago

Hi, I recently ran into a problem using this library where the MediaKeyboardReport.desc() was expanding to:

[
  5u8, 12u8, 9u8, 1u8, 161u8, 1u8, 5u8, 12u8, 25u8, 0u8, 42u8, 20u8, 5u8, 21u8, 0u8,
  39u8, 255u8, 255u8, 0u8, 0u8, 117u8, 16u8, 149u8, 1u8, 129u8, 2u8, 192u8,
]

The expected value is:

[
  5u8, 12u8, 9u8, 1u8, 161u8, 1u8, 5u8, 12u8, 25u8, 0u8, 42u8, 20u8, 5u8, 21u8, 0u8,
  39u8, 255u8, 255u8, 0u8, 0u8, 117u8, 16u8, 149u8, 1u8, 129u8, 0u8, 192u8,
]

... with the second-to-last byte differing. Specifically, the data type flag is being set to variable when the declaration says it's an array. I tracked it down to https://github.com/dtolnay/syn/commit/c09a2e503d5e68af16fa190768d356ea92645f3f ` but I am not very experienced with proc macros so I am not sure if this is a new bug in syn or if the HID macros were just relying on an old bug in syn that was fixed.

twitchyliquid64 commented 3 years ago

Yepp, looks like we might want to pin (#31) till we work out what behaviour we are relying on.

twitchyliquid64 commented 3 years ago

I'm going to guess theres some subtle changes in how we unpack the AST here:

https://github.com/twitchyliquid64/usbd-hid/blob/bb92afa5bc8208c35574edfde645cc2dc91a7006/macros/src/spec.rs#L229

or here:

https://github.com/twitchyliquid64/usbd-hid/blob/bb92afa5bc8208c35574edfde645cc2dc91a7006/macros/src/spec.rs#L364

jyelloz commented 3 years ago

I see, I was a bit short-sighted not to look at the pull requests since I have been trying to bring up USB on the EFM32HG for the past few weeks and may have lost a bit of cognitive abilities in the process, though I eventually got there.

It looks like in 1.0.76 there's just another layer of wrapping around the tree node and the attribute list is reachable. I have put together a proof-of-concept at https://github.com/jyelloz/usbd-hid/commit/420978819d294a7bcd8c3148bf0a5633de0d75c6 that can process the old AST and the new AST but I wonder if there's a simple way to make it descend the tree, recursively finding the first nonempty attribute list to make it immune to extra padding structures in the AST.

twitchyliquid64 commented 3 years ago

may have lost a bit of cognitive abilities in the process, though I eventually got there

Its all good! ^^ I definitely lost brain cells implementing the USB peripheral driver for the samd51, mostly from furiously banging my head into the desk lol

It looks like in 1.0.76 there's just another layer of wrapping around the tree node and the attribute list is reachable. I have put together a proof-of-concept at jyelloz@4209788 that can process the old AST and the new AST but I wonder if there's a simple way to make it descend the tree, recursively finding the first nonempty attribute list to make it immune to extra padding structures in the AST.

From memory I don't think theres a Descender trait or anything we can use - I'm happy to take a PR if you have something that works on both new & old versions of syn

jyelloz commented 3 years ago

I looked around in the documentation and Visit is able to get the job done if we accept that the item_settings macro structure as defined by usbd-hid will not change too much, specifically there will only ever be one list of attributes to parse, then it is really simple. Created pull request #33 as a better implementation than my previous proof-of-concept.