bbcmicrobit / micropython

Port of MicroPython for the BBC micro:bit
https://microbit-micropython.readthedocs.io
Other
602 stars 284 forks source link

Use modular DAL #247

Closed ntoll closed 7 years ago

ntoll commented 8 years ago

@finneyj has explained there's a more "modular" version of the DAL in an upstream branch and that it is this version that will be open-sourced (next week???). We should ensure we cherry pick only those aspects of the DAL we actually use and make use of this awesome new flexibility. :-)

finneyj commented 8 years ago

@ntoll, Yes, James and I have been working hard on a long overdue refactor these last few weeks. We'd not had any time to devote to this until now.

The executive summary is:

This is indeed the version we intend to be live when we open source - although its hot off the press, it's performed pretty well in our regressions tests. No doubt there'll be some issues, but I think it will be a much better base to build on.

In other words, the cherries are now pretty ripe, and ready for picking.

The refactor is in the component-refactor branch of the microbit-dal repo. We'll be looking to bring this into a 2.0.0 release candidate on the master branch as we open source.

ntoll commented 8 years ago

@dpgeorge now that @finneyj has released the source (yay) I wonder if we can cherry pick only the things we need..?

dpgeorge commented 8 years ago

@ntoll (and @markshannon) we need to decide on a course of action. Moving to the modular DAL is definitely something to do, but the question is when. It's not as simple as just updating to the latest version of the DAL, it'll require some work, things will break, will need to be tested and fixed. Also what to do about the filesystem #251?

Basically my question is: v1.0 now, or merge the filesystem and the modular DAL and then tag as v1.0?

ntoll commented 8 years ago

FWIW my vote is 1.0 now, and then work on modular DAL, filesystem and anything else for 1.1.

markshannon commented 8 years ago

I agree with @ntoll

markshannon commented 8 years ago

I've had a look through the new "modular" DAL. It isn't modular. :disappointed: It may be less of a tangle than it used to be, but the code for interacting with the hardware is still dependent on the code for "fiber"s.

However, the DAL is now open source, so we can just rip out the useful bits and discard the rest. :smile:

ntoll commented 8 years ago

I'm pretty certain the folks in Lancaster would welcome PR's and suggestions now that the code is finally open source and unencumbered in terms of collaboration. :-)

markshannon commented 8 years ago

Evidence suggests that they don't welcome PRs: https://github.com/lancaster-university/microbit-dal/pull/42 https://github.com/lancaster-university/microbit-dal/pull/41

dpgeorge commented 8 years ago

I also took a look at the new modular DAL. I was trying to port our code to use v2.0.0-rc3 of the DAL. One problem I ran into is that the MicroBitCompass component now requires a MicroBitStorage (to persist calibration data across resets). But the MicroBitStorage component uses pages 17 and 19 (at 17k and 19k) to save its data. That is incompatible with our build because that region of flash is used by our firmware (for those using BLE those pages are free).

@finneyj any ideas?

finneyj commented 8 years ago

@dpgeorge Thanks for raising this. We reviewed the location that we were using to store persistent data in v2.0.0, and found that one of pages allocated to the BLE bootloader was actually reserved for persistent application data - so free for us all to use. Hence, took the opportunity with the change in major version to start using this space for a general purpose key/value store, and free up an additional 1K of FLASH.

I certainly don't want this to be a stumbling block for anyone, but it would also be good to reclaim as much FLASH wherever we can.

I presume the FLASH memory you need for firmware needs to be contiguous?

dpgeorge commented 8 years ago

I presume the FLASH memory you need for firmware needs to be contiguous?

Not really. But we need 200k+ of flash for the firmware so splitting it in 2 pieces (below and above 17-19k) will be rather difficult.

finneyj commented 8 years ago

I see what you mean. Sorry, thought we'd agreed this was ok in principle in issue #174, and that a 1K shift in the page used wouldn't cause too much of a problem if we did this as part of a major version change (my bad).

I think we really only need 1 page to hold the data, and one as a default scratch page if we're not running builds with a file system (otherwise we need 1K of RAM to perform updates, and this is not ideal for obvious reasons). I've intentionally held off tagging a full 2.0 release to buy us some flexibility in resolving issues like these, so am open to suggestions if you have ideas?

I guess obvious possibilities are to move the storage page(s) to somewhere less invasive (e.g. closer to the top/bottom of FLASH memory). This could give us compatibility issues with the ARM/gcc init code at the bottom of FLASH and Nordic BLE bootloader at the top, but may be resolvable through tweaks to the mbed target or BLE bootloader... but probably not trivially.

dpgeorge commented 8 years ago

thought we'd agreed this was ok in principle in issue #174,

I thought the resolution of that issue was to use the upper 24k of flash for this purpose? And then @markshannon suggested to simply declare a symbol in the C code (a variable worth 1024 bytes) that is allocated by the linker, and then just use the address of that for the flash page.

Alternatives are to make the MicroBitStorage component optional, or make it pluggable but a custom component that provides the requisite key/value storage (which could just be a dummy implementation, or could read/write to a file in the local filesystem).

markshannon commented 8 years ago

I don't think it needs to be that complex. To decouple any two components (in this case the compass and persistent storage), just make sure that each depends on the other's interface and not its implementation.

Here is a simple interface for the persistent storage:

int persistent_put(char *keychars, unsigned keylen, void *data, unsigned datalen);
int persistent_get(char *keychars, unsigned keylen, void **data, unsigned *datalen);

If the compass depends only on the interface, then it becomes easy to link in a different storage component.

finneyj commented 8 years ago

Sorry guys - missed the end of the discussion on #174.

Bringing out an interface definition would address issues of having multiple implementations, but wouldn't necessarily provide a consistent experience for users. e.g. if a user swaps languages, then persistent storage can't be shared between those languages (which is not terrible, but certainly not ideal either).

Applications I'm aware of thus far using key/value storage functionality are:

I'm happy to bring out a key/value storage interface to support different implementations - that's harmless, and we'll likely evolve in that general direction as the diversity micro:bit hardware grows over time.

But I do wonder about the consequences on end users of not having a shared way of storing this information. i.e. if we all want the same/similar info in all languages, would it not be better for us to find a way to join up our efforts?

dpgeorge commented 8 years ago

if a user swaps languages, then persistent storage can't be shared between those languages

How would this work in practice, given that the entire flash is erased when changing languages (at least to/from MicroPython)?

finneyj commented 8 years ago

BBC asked @alex21k to work with BBC and ARM to draft up a tailored USB interface firmware that can (optionally) persist small sections of the nrf51822 memory across USB reprogramming operations. Maintaining small amounts of data across reflashes keeps cropping up as a requirement on and off... hence the MicroBitStorage key/value pair component looking how it does (i.e. an absolute block address is much easier to deal with).

If MicroPython is best suited by an alternative implementation, we'll just bring out an interface and we're fine. Just don't want you guys accidentally left out in the cold, if other languages have a shared model.

dpgeorge commented 8 years ago

BBC asked @alex21k to work with BBC and ARM to draft up a tailored USB interface firmware that can (optionally) persist small sections of the nrf51822 memory across USB reprogramming operations.

That sounds pretty useful, and something we can also benefit from. For us it would be cool to be able to persist the local filesystem across flashes (eg the upper 24k of flash). But I'm guessing that it won't be that configurable and there'll only be 1-2 flash pages dedicated to this feature?

jamesadevine commented 8 years ago

But the MicroBitStorage component uses pages 17 and 19 (at 17k and 19k) to save its data

@dpgeorge I may have missed something in the discussion, but it's 17k from the end of flash:

(256 - MICROBIT_STORAGE_STORE_PAGE_OFFSET) * 1024 = 0x3bc00

Where MICROBIT_STORAGE_STORE_PAGE_OFFSET is 17.

finneyj commented 8 years ago

Yep - useful for everyone.

At the moment we're constrained to persisting just a few blocks, by design. There's a tradeoff between either bulk erasing the nrf51822 and holding a few blocks in the RAM in the KL26 interface chip, or performing a block by block erase of the nrf51822. The former is much faster - block by block erasing adds quite a long time onto the programming time of the device (can't remember exact figures, but from memory it was something like an additional 12-14 seconds). As USB reprogramming is such a common use-case, this is quite a big impact.

We'd also considered persisting a file system. This would need to be based on block by block erasing, so we'd need to look at something configurable in the firmware. At the moment we have the block erase approach implemented, with BLE bonding data and MicroBitStorage key/value blocks penciled in for persistence. The former as a specific necessity, the latter as a more general purpose mechanism.

All things are possible though, if we can muster the resources. :-)

dpgeorge commented 8 years ago

I may have missed something in the discussion, but it's 17k from the end of flash:

Ah, I see. Then that does change things. But now it clashes with our filesystem data, which lives in the upper 30k of flash.

If the new firmware flashing system is going to leave the blocks at (256-17) and (256-19) as-is then we should take advantage of that. Can we convince ARM to make it so that the upper, say, 24k is left alone?

dpgeorge commented 8 years ago

There's a tradeoff between either bulk erasing the nrf51822 and holding a few blocks in the RAM in the KL26 interface chip, or performing a block by block erase of the nrf51822.

Ok, that does provide some big constraints. An extra 12-14 seconds waiting for it to program is indeed a long time.

finneyj commented 8 years ago

Indeed - looking at the nrf51822 data sheet, erase time per block is about 25ms, which would yield an additional erase time of about 6.4 seconds. So I suspect the number I was remembering for the block by block algorithm is the total reflash time, not the delta. i.e. it would probably double the programming time, from about 6 or 7 seconds to around 14 seconds. Still a very significant impact on the common case though...

whaleygeek commented 8 years ago

I'm sure you guys are all over this anyway, but can I make a call for this to be a 100% backwards compatible change (i.e. somehow fail safe if the interface firmware and generated .hex file are not of the same breed?)

In particular, the two obvious use cases are: 1) if a child has a micro:bit with old interface firmware loaded, and they load in code they have just written in any of the 4 languages, that assumes the new runtime image, it should work.

2) if a child has a micro:bit that has updated interface firmware, and they load in code from a hex file from any of the 4 languages that they cached on a memory stick from months ago, it should still work.

It's natural for some features to be 'degraded' where support is not available in the underlying platform, but I would still expect the script to be able to load and generally run without crashing horribly.

otherwise there will need to be some (BBC managed?) process, toolset, workflow and set of instructions, to manage a more automated upgrading of older micro:bits to the new firmware.

carlosperate commented 8 years ago

But the MicroBitStorage component uses pages 17 and 19 (at 17k and 19k) to save its data

@dpgeorge I may have missed something in the discussion, but it's 17k from the end of flash:

(256 - MICROBIT_STORAGE_STORE_PAGE_OFFSET) * 1024 = 0x3bc00

Where MICROBIT_STORAGE_STORE_PAGE_OFFSET is 17.

I though the softdevice was stored at the end of Flash? I understand micropython uses the gcc-nosd target, but if this is a DAL implementation doesn't that clash? Is this the BLE bootloader space @finneyj was mentioning before?

Also, does that mean this uses pages 256-17 AND 256-19, or pages 256-19 to 256-17 (which would include 256-18).

finneyj commented 8 years ago

Hi @carlosperate

Nearly, but not quite! Nordic SoftDevice (which provides Nordic's bluetooth stack among other things) is designed to be resident at the bottom of FLASH memory. There are various different SoftDevice images, that provides different levels of functionality. We currently go for the smallest one, S110, which provides only peripheral mode operation. This still clocks in at about 98K static FLASH footprint, and ~12K SRAM overhead (8K static + 2K for mbed/nordic SDKs + 1.8K on the stack when a big interrupt goes off).

To support over the air programming, there's also a memory resident Nordic BLE Bootloader that's an additional 16K, and is designed to be memory resident at the top of memory. Under this, there's an additional two pages reserved by SoftDevice: one to hold security key information used for Bluetooth bonds, and one designated for application data. We also drop on a page after this for use as a scratch page for the key/value pair storage described above.

So for BLE enabled builds, this gives us:

256
.
.                Nordic BLE Bootloader
.
256-16
256-17      key/value pair registry (MicroBitStorage)
256-18      Nordic BLE bond table
256-19      <scratch page>
.
.  <user code/libs>
.
96
.
.                Nordic SoftDevice
.
0

The pages we're currently preserving are 256-17 and 256-18.

p.s. Thanks for asking - this reminds me to put a memory map in our docs...

finneyj commented 8 years ago

@whaleygeek Good to see you on the ball as ever!

Yes, the intention is for 100% backward compatibility, with no dependencies on runtime version.

For the first of your two use cases (old firmware, new runtime), then the behaviour would simply be that persistent data would in fact be not persistent at all. Firmware could be updated at any later date to enable persistence. The second case (new firmware, old HEX) can be handled by only persisting data blocks if they are not present in the HEX file being programmed. So if an old HEX file happens to use the now reserved blocks, it will still be programmed correctly - and as the version of the runtime that the program was built against will be part of the HEX file, there won't be any issues when the program is run. The live version of the runtime already avoids these blocks though, so this should not be an issue for Blocks/TD/CK-JS/C. I think you guys would know better than I if there would be any implications for micropython, but the worst that could happen is that persistent storage doesn't work.

BBC are indeed managing (Mark Joyce), as this is related to their Wellcome and accessibility workstreams. BBC are losing resources fast though, so good for us to start to use community led mechanisms in the future though methinks...

dpgeorge commented 8 years ago

@jamesadevine just to confirm with the new DAL: the way to provide custom config options (eg disable BLE) is to use config.json, and that the MicroBitCustomConfig.h file (which we currently use) is now obsolete?

jamesadevine commented 8 years ago

@dpgeorge Yes, the custom config mechanism should now be considered deprecated 😄

finneyj commented 8 years ago

The MicroBitCustomConfig.h approach should still work, but quite a few folks reported benefits of the config.json approach (primarily transparency and scope), so we're migrating to that for yotta based builds.

i.e. with config.json you don't need to #include MicroBitCustomConfig.h in your app to guarantee consistent options between your app and microbit-dal modules, and the yotta config options seem to pervade through the entire build, so can be used e.g. by @remay who was wanting to tweak other bits of code based on config options.

carlosperate commented 8 years ago

(btw, thanks for the great explanation @finneyj, soft device and bootloader maps are a lot clearer now, would be great to have it in the docs as well as you suggested :)

markshannon commented 7 years ago

Sadly, it turns out that the "modular" DAL isn't modular at all. So we won't be using it. If a newer genuinely modular DAL does appear, then we should revisit this decision. But, given that a DAL suitable for MicroPython seems unlikely, I'm closing this issue.