Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firmware modified to support CODAL #12

Merged
merged 7 commits into from
Apr 21, 2022

Conversation

martinwork
Copy link
Contributor

microbit-v2-samples is included as a submodule to build for V2.

After cloning the repo, to build V2 firmware microbit-firmata/microbit-v2-samples/MICROBIT.hex:
cd microbit-firmata
git submodule update --init
cd firmware
python buildv2.py

The desktop client expects some event source numbers that are different in V1 and V2. The CODAL firmware translates the V2 source numbers to the V1 ones in eventToExternal(). This is OK because only a few events are supported, but could become very awkward if more events are supported.

The code supports macro FIRMATA_USE_UBIT to test V1 and V2 compiling either uBit, or just the required components (the default), but with neither option starting the scheduler. The streaming test speeds are affected very slightly (0.1% ?) by compiling uBit.

@jaustin
Copy link
Contributor

jaustin commented Aug 11, 2021

@martinwork it sounds like it might be a better step then to update the numbers used by the desktop software for the events and align DAL and CODAL? Is this something @finneyj did for MakeCode too? Or is there some translation there?

@finneyj
Copy link

finneyj commented Aug 11, 2021

I thought we had aligned most of them already - do we know which ones are different?

@jaustin
Copy link
Contributor

jaustin commented Aug 11, 2021

My guess is that the firmata build here for V1 pre-dates the microbit-dal update, so if we rebuild both V1 and V2 hexes against latest microbit-dal and CODAL, and then update the event numbers we might have everything "just line up again" ?

@martinwork
Copy link
Contributor Author

@jaustin @finneyj

The external clients are expecting the DAL numbers:
https://github.com/microbit-foundation/microbit-firmata/blob/master/client/MBFirmataClient.js#L322

I was assuming there were clients we didn't have control over, so it would be CODAL changing to match DAL.

What DAL update aligned the IDs? Is it a difference between DAL master and the branch used for MakeCode?

These are the IDs the V2 firmware is translating from CODAL to the one's expected by the client:
https://github.com/microbit-foundation/microbit-firmata/pull/12/files#diff-3586b71736669d8789c831e156f52d0f5e6ba35a8c8b1371e402313746f2ce6fR141

@martinwork
Copy link
Contributor Author

The V1 yotta module.json points to "lancaster-university/microbit#v2.1.1"

@finneyj
Copy link

finneyj commented Aug 11, 2021

I think it's this one:
lancaster-university/microbit-dal@76538ef

Can't remember why we did this precisely - but was at the request of makeCode team. Probably because on v2 we use pxt-common-packages much more...

@martinwork
Copy link
Contributor Author

That is in the js-event-semantics branch, right?

@jaustin Should I point the firmata V1 build at v2.2.0-rc6, and update the constants in MBFirmataClient.js?

@finneyj
Copy link

finneyj commented Aug 11, 2021

Yes, MakeCode for micro:bit v1 is running off that branch AFAIK.

@jaustin
Copy link
Contributor

jaustin commented Aug 16, 2021

@martinwork yes, I think so. We'll need to make a new release that's a breaking change with these IDs but I think the only sane way to do it is to align V1 and V2 on the same IDs, and I think the only way we could do that sanely for pxt was by bringing V1 up to V2 so that's what had to happen. @finneyj I guess this will affect other users of CODAL - do we need to think about a compat system? Is there a 1:1 mapping for V1:V2 IDs that we can use? What's the overlap between the V2 range and V1 range? Could V2 additionally support all V1 IDs mapped to the new namespace?

@martinwork
Copy link
Contributor Author

@jaustin I don't know what other clients are out there, or if they are all based on MBFirmataClient.js. If a client is changed to use new numbers, old firmware will stop working.

We can avoid any need for changes by simply deciding / defining that the message MB_REPORT_EVENT will only ever be used for the current 7 events with the V1 numbers. If required in the future, a new message could support other raw events.

Perhaps all we need is a comment to that effect in the firmware code.

@jaustin
Copy link
Contributor

jaustin commented Aug 17, 2021

@epeach could you let us know whether it'd be an issue at your end to renumber the events in line with the newer numbers? Although we could maintain the older ones, @martinwork as I think there are few users of this so far, I'd be inclined to introduce things that look like 'legacy' at this point and perhaps do a protocol rev that allows anyone who is using it to determine that the numbers have changed?

@martinwork
Copy link
Contributor Author

@jaustin I didn't realise that the number of simple messages is quite limited.
https://github.com/firmata/protocol/blob/master/protocol.md
https://github.com/microbit-foundation/microbit-firmata/blob/master/firmware/source/mbFirmata.h#L56

We can change the firmware version:
https://github.com/microbit-foundation/microbit-firmata/blob/master/firmware/source/mbFirmata.cpp#L143

MBFirmataClient.js doesn't use the firmware version at the moment apart from storing it in a string.
https://github.com/microbit-foundation/microbit-firmata/blob/master/client/MBFirmataClient.js#L290

So MBFirmataClient.js and any other clients will have to store the firmware version numbers and expect different IDs starting with v1.1.

Update DAL version tag in firmware/module.json
Update Firmware version to 1.1: major=1, minor = 1
Some firmware event IDs depend on the firmware version: MICROBIT_ID_DISPLAY/GESTURE/IO_P0/IO_P1/IO_P2
MBFirmataClient.js - add variables for the version dependent IDs
mbTests.js - use the variable mb.MICROBIT_ID_GESTURE
mbTests.js - fix String/Number scroll tests
README.md - add V2 build intructions
@martinwork
Copy link
Contributor Author

martinwork commented Aug 18, 2021

@jaustin I've added commit 771f2d8

  • use DAL v2.2.0-rc6
  • move varying event ID code to client
  • bug fixes in the client and tests
  • V2 build instructions in README
  • Universal microbit-firmata-v1.1.hex in precompiled folder

@jaustin
Copy link
Contributor

jaustin commented Aug 18, 2021

@martinwork
Copy link
Contributor Author

@jaustin @epeach

Client code must be updated to use the event numbers that vary between v1.0 and v1.1.
This PR updates MBFirmataClient.js for this.

Any event handler registered with MBFirmataClient.js should use the variables set here:

this.MICROBIT_ID_DISPLAY = 7;

Like this:

if (mb.MICROBIT_ID_GESTURE == sourceID) {

@epeach
Copy link
Contributor

epeach commented Aug 18, 2021

Hi there - not a problem on our end to renumber the events. I tried the v1.1.hex you linked and after a quick look at a sample project, it appears to be working as expected. Let me know if there's any other questions!

@martinwork
Copy link
Contributor Author

Thanks @epeach. I have fixed the ID in the "fake" animation complete event the firmware sends when processing a display request while the display is disabled, and updated the precompiled hex.

@JohnVidler
Copy link
Collaborator

I've checked this out and it seems to work just fine; so happy to merge.

@JohnVidler JohnVidler merged commit 0ab338c into microbit-foundation:master Apr 21, 2022
Comment on lines +204 to +205
if ('9903' == id) return '2.0';
if ('9904' == id) return '2.0';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnVidler We'll need to add board IDs 9905 and 9906 here as well for V2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants