lancaster-university / codal-microbit-v2

CODAL target for the micro:bit v2.x series of devices
MIT License
44 stars 52 forks source link

DEVICE_ID_USB clash with MICROBIT_ID_PARTIAL_FLASHING ? #96

Closed martinwork closed 1 year ago

martinwork commented 3 years ago

Will new #define DEVICE_ID_USB 36 create conflict with #define MICROBIT_ID_PARTIAL_FLASHING 36?

https://github.com/lancaster-university/codal-core/blob/master/inc/core/CodalComponent.h#L70

https://github.com/lancaster-university/codal-microbit-v2/blob/master/inc/compat/MicroBitCompat.h#L359

jamesadevine commented 3 years ago

Good catch Martin. I don't think this will trip us up on the micro:bit as there is no ability for it to communicate over USB directly. This will certainly catch us out if we decide to operate the partial flashing service at the same time as USB on a different board.

I'm more than happy to shift DEVICE_ID_USB somewhere else. However, the micro:bit defines should be using the generally allocatable range as they are somewhat specific to micro:bit at this time. What value would you recommend for DEVICE_ID_USB?

@finneyj we should contemplate more thoroughly on a way to ensure there are no ID collisions between components. It might be cool to have a way to ensure no ID collisions exist at compile time...

martinwork commented 3 years ago

I don't know what value to suggest for DEVICE_ID_USB. What is the "generally allocatable range"?

Related: https://github.com/microbit-foundation/codal-microbit/issues/77.

jamesadevine commented 3 years ago

https://github.com/lancaster-university/codal-core/blob/master/inc/core/CodalComponent.h#L85 4000+

martinwork commented 3 years ago

@jamesadevine @finneyj

Should CodalComponent.h define, something like

#define DEVICE_ID_TARGET_MIN 4000
#define DEVICE_ID_TARGET_MAX 4999

#define DEVICE_ID_USER_MIN 5000
#define DEVICE_ID_USER_MAX 5999

So we might redefine IDs 36 to 44 in MicroBitCompat.h like:

#define MICROBIT_ID_PARTIAL_FLASHING (DEVICE_ID_TARGET_MIN + 0)
#define MICROBIT_ID_POWER_MANAGER  (DEVICE_ID_TARGET_MIN + 1)
martinwork commented 3 years ago

A range of IDs is reserved for jacdac...

https://github.com/lancaster-university/codal-core/blob/master/inc/core/CodalComponent.h#L83 // jacadac reserved from 3000 - 4000

define DEVICE_ID_JD_DYNAMIC_ID 3000

Are there clashes with jacdac in these IDs above 3000?

https://github.com/lancaster-university/codal-microbit-v2/blob/4f76deafe67268227b7b4b9c2f471595e91ab2a5/model/MicroBit.h#L318

https://github.com/lancaster-university/codal-microbit-v2/blob/111e4cf829fdfea25e301375643d1bad71fb7070/inc/SoundEmojiSynthesizer.h#L46

https://github.com/lancaster-university/codal-microbit-v2/blob/111e4cf829fdfea25e301375643d1bad71fb7070/inc/Mixer2.h#L42

Beware IDs not defined with all the others...

https://github.com/lancaster-university/microbit-dal/blob/602153e9199c28c08fd2561ce7b43bf64e9e7394/inc/bluetooth/ExternalEvents.h#L33 https://github.com/lancaster-university/codal-microbit-v2/blob/111e4cf829fdfea25e301375643d1bad71fb7070/inc/bluetooth/ExternalEvents.h#L33

MakeCode has used IDs above 4000

https://github.com/microsoft/pxt-common-packages/blob/58f1ea42b83f09e1c76ccfe637cbc90b247b0fc3/libs/core/pxt.h#L115

define DEVICE_ID_BUTTON_SLIDE 3000

define DEVICE_ID_MICROPHONE 3001

define DEVICE_ID_FIRST_BUTTON 4000

define DEVICE_ID_FIRST_TOUCHBUTTON 4100

define PXT_INTERNAL_KEY_UP 2050

define PXT_INTERNAL_KEY_DOWN 2051

https://github.com/microsoft/pxt-common-packages/blob/3e54bbf9805405dd2da1930c6fea68867f59b9ce/libs/pulse/pulse.h#L15

define PULSE_IR_COMPONENT_ID 0x2042 [8258]

define PULSE_CABLE_COMPONENT_ID 0x2043 [8259]

martinwork commented 1 year ago

The issue in the title has been fixed by https://github.com/lancaster-university/codal-microbit-v2/commit/9223c30d89bc615be8ca1bc1c439eae31c01b337, but I think the list in the previous https://github.com/lancaster-university/codal-microbit-v2/issues/96#issuecomment-898884793 has some other possible current or future clashes.

microbit-carlos commented 1 year ago

Thanks Martin! If you think the IDs from your previous comment could have clashes, could you open a new issue to make sure we check them at some point?