search

lancaster-university / microbit-dal

http://lancaster-university.github.io/microbit-docs
Other
254 stars 130 forks source link

Use config.json to enable / disable Partial Flashing Service #382

Closed microbit-sam closed 6 years ago

microbit-sam commented 6 years ago

This PR allows partial flashing to be enabled by a config.json setting

As the service is going to always exist with the partial flashing capabilities enabled / disabled by the flag should it be named something else. I think Martin suggested Utility Service as it contains the reboot / information characteristics too

@martinwork @finneyj @jaustin

martinwork commented 6 years ago

I think I hinted at toolbox, but utility sounds OK!

This seems to just wrap around FLASH_DATA command and return ABCDEF. By the time the client app sends FLASH_DATA, it has gone through a sequence of other commands, including possibly rebooting to pairing mode - an unnecessary delay of several seconds if partial flashing isn't going to work.

Could partial flashing be locked out by returning an invalid hash in the dal region info, or zeros for the makecode region info, or both since both come from the makecode magic info. These are the first steps in the sequence. I think the DAL region hash is currently zero when no makecode magic is found, which makes sense, but the makecode region has valid addresses, which doesn't make sense.

The service without partial flashing should avoid consuming RAM for the variables that are only used for partial flashing. Really these variables should be allocated only in pairing mode, since partial flashing is only done in pairing mode.

pelikhan commented 6 years ago

@microbit-sam anything to do on the MakeCode side?

jaustin commented 6 years ago

What's the rationale for keeping the service there but not allowing flashing? Isn't it more straightforward and useful for config if we just ditch the service altogether when it's not enabled? I prefer the idea that 'if the service is there it works as intended' but I suspect there's a reason I haven't grokked that you want to keep it around in a slightly less complete form? @microbit-sam

microbit-sam commented 6 years ago

In my mind I had it that it would allow a C++ program to be remotely booted into pairing mode and then could enter DFU mode and be flashed

Although actually.. I think if the PartialFlashingService exists in the application mode then the DFU service will also be there without needing to enter pairing mode

@martinwork do you think it's worth keeping the rest of the commands (info/reboot.)? Perhaps in the future we should create a separate utility/toolbox service rather than piggybacking on this one

microbit-sam commented 6 years ago

@pelikhan yes please - could "partial_flashing": 1 be added to the yotta config in MakeCode e.g.

    "microbit-dal": {
        "bluetooth": {
                    "enabled": 0,
                    "private_addressing": 0,
                    "advertising_timeout": 0,
                    "tx_power": 6,
                    "dfu_service": 1,
                    "device_info_service": 1,
                    "eddystone_url": 1,
                    "eddystone_uid": 1,
                    "open": 0,
                    "pairing_mode": 1,
                    "whitelist": 1,
                    "security_level": "SECURITY_MODE_ENCRYPTION_NO_MITM",
                    "partial_flashing": 0
                },
        "gatt_table_size": "0x300"
    }

Although perhaps wait for the conclusion of this PR

Thanks!

jaustin commented 6 years ago

...Right - I think the option you added with the partial flashing series that allows code to enter pairing mode is useful and should remain (though that's in ubit anyway), but I think the actual PartialFlashingService isn't useful? The DFU service can be used to get into pairing mode for a BLE enabled application.

martinwork commented 6 years ago

The DFU service cannot be used to get into pairing mode. Pairing mode is not needed for a BLE enabled application to flash.

As a workaround for the problem created by flashing a C++ program over a makecode project, perhaps the best simple immediate fix is a config that completely removes the PF service. But the only requirement for the fix is that the service disables partial flashing and lets the client app know the situation.

I thought all the features of the PF service should have been implemented as configurable extensions to the DFU service, in order to minimise the RAM and GATT table footprint. So I wouldn't suggest creating a separate service for the "utility" features. I have always said that the separate PF service should be controlled by it's own config, so that it doesn't get included by default and break existing code, and that it should only ever allocate its PF variables in pairing mode. With the PF service, it looks like the other services no longer all fit in the GATT table.

The ability to discover if micro:bit is in pairing mode has been requested elsewhere. The ability to reboot to either pairing or application mode could be useful. I understand that the region requests are part of a long term plan beyond the current partial flashing protocol - that's why they are a bit clunky for use in the PF process.

One reason given for not adding the PF service features to the DFU service was the fear that might break existing software. If we specify the PF service now as a toolbox with configurable parts such that clients can discover which features are supported and aren't broken by adding new features, we could have a future route that might enable new features to be added without the overhead of adding yet another new separate service.

Perhaps it should be documented that the inclusion of the PF service is not a "makecode flag".

pelikhan commented 6 years ago

Good to know. Will wait for the PR.

From: Sam Kent notifications@github.com Sent: Wednesday, September 5, 2018 4:08 PM To: lancaster-university/microbit-dal microbit-dal@noreply.github.com Cc: Peli de Halleux jhalleux@microsoft.com; Mention mention@noreply.github.com Subject: Re: [lancaster-university/microbit-dal] Partial Flashing disabled by default. Enabled for MakeCode builds (#382)

@pelikhanhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpelikhan&data=02%7C01%7Cjhalleux%40microsoft.com%7Cd620d408b9c64658f18a08d613846f2e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636717856852898378&sdata=S8pZzLMFfTvNpOz33aA%2B%2F3BrT6f%2B8H47opqZFExIDUQ%3D&reserved=0 yes please - could "partial_flashing": 1 be added to the yotta config in MakeCode e.g.

"microbit-dal": {

    "bluetooth": {

                "enabled": 0,

                "private_addressing": 0,

                "advertising_timeout": 0,

                "tx_power": 6,

                "dfu_service": 1,

                "device_info_service": 1,

                "eddystone_url": 1,

                "eddystone_uid": 1,

                "open": 0,

                "pairing_mode": 1,

                "whitelist": 1,

                "security_level": "SECURITY_MODE_ENCRYPTION_NO_MITM",

                "partial_flashing": 0

            },

    "gatt_table_size": "0x300"

}

Although perhaps wait for the conclusion of this PR

Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flancaster-university%2Fmicrobit-dal%2Fpull%2F382%23issuecomment-418910053&data=02%7C01%7Cjhalleux%40microsoft.com%7Cd620d408b9c64658f18a08d613846f2e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636717856852898378&sdata=1FYsGWPajWD95t9V9hmi%2FHSfQermrpcatIeH07xFSbQ%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAD-4KQ2PRJe4XO72cabVEDdDUozfik8Oks5uYFlTgaJpZM4WbRvz&data=02%7C01%7Cjhalleux%40microsoft.com%7Cd620d408b9c64658f18a08d613846f2e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636717856852908386&sdata=s2EvKJyoNTGy9HVvY4n1JZaHSMjpgSbQkjG8EY%2B2VLM%3D&reserved=0.

microbit-sam commented 6 years ago

I think Joe and I now have a memory fix allowing us to use the full size GATT table again, so hopefully the extra service is less of a problem

It would be nice to have it as an extension of the DFU service, but the initial worry was that it could create some incompatibilities with existing clients.. I'm not sure who / what, but it's definitely a possibility

I've changed this branch to completely enable / disable the Partial Flashing Service.

Would this be reasonable for now? With the possibility of morphing into a toolbox / merging into the DFU service / something else in the future. This seems a fairly important feature to include to prevent users with C++ programs trying to partially flash their programs and getting stuck, so it would be nice to get it into the next DAL release

finneyj commented 6 years ago

Thanks @microbit-sam

This looks like a very sensible way forward. No reason not to have a compile time option here. We can always add something more sophisticated in the future if we have the resources...

merging this into dal-integration.

microbit-sam commented 5 years ago

Thanks Joe! @pelikhan When MakeCode is updated to use the recent dal-integration-5 tag, please could you enable partial flashing by setting the flag in the Yotta config.json

e.g.

"microbit-dal": {
        "bluetooth": {
                    "partial_flashing": 1,
                     ...
                },
         ...
    }
pelikhan commented 5 years ago

@gujenkin