Architeuthis-Flux / Jumperless

A jumperless breadboard
Other
738 stars 36 forks source link

"b" macros in JumperlessDefines conflict with variable names in TinyUSB library #16

Closed dupontgu closed 6 months ago

dupontgu commented 6 months ago

When I load in the JumperlessNano project via platformio, the build seems to fail because the pin #defines in JumperlessDefinesRP2040:

#define b1 31
#define b2 32
#define b3 33
// ...

conflict with parameter names in a utility function in tusb_common.h (from the TinyUSB library):

TU_ATTR_ALWAYS_INLINE static inline uint32_t tu_u32(uint8_t b3, uint8_t b2, uint8_t b1, uint8_t b0)
{
  return ( ((uint32_t) b3) << 24) | ( ((uint32_t) b2) << 16) | ( ((uint32_t) b1) << 8) | b0;
}

I've attached the full error output - this may be due to a configuration issue on my end? No idea. But I pulled fresh and this was the first issue I saw.

Workaroud:

I simply renamed the parameters in the tusb util function since it was quicker than changing the macro.

TU_ATTR_ALWAYS_INLINE static inline uint32_t tu_u32(uint8_t byte3, uint8_t byte2, uint8_t byte1, uint8_t bbyte0)
{
  return ( ((uint32_t) byte3) << 24) | ( ((uint32_t) byte2) << 16) | ( ((uint32_t) byte1) << 8) | byte0;
}

This project is incredible and I am having so much fun!!!

jumperless_err.txt

Architeuthis-Flux commented 6 months ago

Ha, I did that exact same workaround (I even picked the same names) and forgot to make it so other people wouldn't have to do it too.

I just need to decide whether to put the revised library in the source or change the defines. Any thoughts?

dupontgu commented 6 months ago

Though it's a bit more effort, I would probably change the defines. If you embed the lib, will cost ya time down the road if you ever wanna update. And fwiw I think most c++ styleguides recommend ALL_CAPS_AND_UNDERSCORE case for macros.

Architeuthis-Flux commented 6 months ago

Cool, turns out those defines were only used in one other file, MatrixStateRP2040.cpp, so I just changed them to follow the style guide as you suggested. Now they're TOP_1 and BOTTOM_1 instead of t1 and b1. Thanks for catching that!