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

fix NRF.restart() on SDK14 and up #1802

Merged
merged 1 commit into from Apr 20, 2020
Merged

Conversation

fanoush
Copy link
Contributor

@fanoush fanoush commented Apr 19, 2020

fix for

>NRF.restart()
NRF ERROR 0x8 at targets/nrf5x/bluetooth.c:2219
REBOOTING.

this is caused by unmatched proper nrf_sdh_disable_request() call and triggering this

    if (m_nrf_sdh_enabled)
    {
        return NRF_ERROR_INVALID_STATE;
    }

in nrf_sdh_enable_request() in targetlibs/nrf5x_14/components/softdevice/common/nrf_sdh.c

This helps me on SDK14 and bluetooth seems to be working after restart.

BTW I tried adding hook to run JS code when softdevice is disabled (to e.g. erase/rewrite UICR to relocate bootloader or write to other protected registers) and it works! I have added quick hack like

 jsble_kill();
  jsvUnLock(jspEvaluate("if(typeof(NRF.onRestart)=='function')NRF.onRestart();",true));
  jsble_init();

to jsble_restart_softdevice() and can do basically anything there while softdevice is temporarily disabled :-)

fix for
>NRF.restart()
NRF ERROR 0x8 at targets/nrf5x/bluetooth.c:2219
REBOOTING.
@fanoush fanoush changed the title fix NRF.restart() on SDK14 (and up?) fix NRF.restart() on SDK14 and up Apr 19, 2020
@fanoush
Copy link
Contributor Author

fanoush commented Apr 19, 2020

Also tested on Particle XENON - 52840 with SDK15 and NRF.restart() now works there too without reboot.

@gfwilliams
Copy link
Member

Great - thanks!

Did you spot E.on('kill',...)? That might do basically the same as NRF.onRestart? Or if it doesn't maybe that's a bug ;)

@gfwilliams gfwilliams merged commit c998d8b into espruino:master Apr 20, 2020
@fanoush fanoush deleted the patch-1 branch April 20, 2020 08:33
@fanoush
Copy link
Contributor Author

fanoush commented Apr 20, 2020

Did you spot E.on('kill',...)? That might do basically the same as NRF.onRestart? Or if it doesn't maybe that's a bug ;)

Thanks for the tip, I tried it and it seems to do nothing. Should this work?

>E.on('kill',function(){poke32(0x4000051c,2);})
=undefined
>NRF.restart()
=undefined
>peek32(0x4000051c)
=0

this is poking GPREGRET which is protected for writes and normally it reboots. Even if it would be called I need it in right moment when softdevice is disabled so the memory protection is off.

How would the event be fired exactly? There is this part here https://github.com/espruino/Espruino/blob/master/targets/nrf5x/bluetooth.c#L2468 where the jsble_kill is called (the method itself is few lines above) and I don't see how it would trigger it.

@gfwilliams
Copy link
Member

Sorry, I was getting confused between this and http://www.espruino.com/Reference#l_E_reboot

So yeah, there isn't anything. I guess the question is why do you need it? Is it to re-add something after the stack has rebooted, or to fiddle with something that the stack was protecting while active?

We could add something like onRestart as you suggest, but it's super-dangerous. Likely any print statement or attempt to call NRF.* would cause a crash?

@fanoush
Copy link
Contributor Author

fanoush commented Apr 20, 2020

yes, to fiddle with something that the stack was protecting, like if you would not compile BLUETOOTH module in at all, then you would also have 'superpowers' like that. Actually I was quite surprised it works and is far more reliable than I was expecting. I have later actually added NRF.stop() and NRF.start() that would shutdown softdevice completely for longer time (i.e. splitted restart to two calls). I have guarded NRF.stop() by

!jsble_has_connection() && (bleStatus&BLE_IS_SLEEPING)

so you would have to call NRF.sleep() first. And with this the interactive console (over serial) actually kept working without crash after softdevice was disabled, I only noticed system time was stopped. True that BLE related NRF. methods would fail (and some even reboot? unless guarded by some flag, I have added BleStatus enum 1<<31 = BLE_OFF for this). And there are also some different implementations all over the code based on #if defined(BLUETOOTH) but not actually that much so I guess adding some semi runtime checks like if (BLUETOOTH_ENABLED || sd_enabled) to same places would make it working and with BLUETOOTH_ENABLED=0 would result in same code as now.

As for whether this is worth it - it probably isn't :-)

OTOH the code path with BLUETOOTH disabled should be kept working anyway so except guarding NRF ble calls by some flag (some are already guarded, some work even with sd disabled) it is not that much extra work. BTW can whole NRF object at runtime be completely removed/disabled/switched to other implementation easily? then it could be E.startSD() E.stopSD() and whole NRF object would be unavailable if stopped. There would be advantage if we also had resizable variables so with stopped SD you could have more of them. Also is related to dynamic MTU size reconfiguration.

@gfwilliams
Copy link
Member

Wow, that's impressive! What is it you're doing while BLE is off?

As for whether this is worth it - it probably isn't

I'm afraid right now that's probably my feeling - at least for master. way too many ways to break things :) I'm happy to have it as a branch though.

OTOH the code path with BLUETOOTH disabled should be kept working anyway

Yes, totally. If you've got some changes to help keep that the case I'd be very interested.

I'm just not convinced about runtime switching - I'm really starting to run low in available flash now, so I'm not sure it'd be worth removing other features to get this one working.

whole NRF object would be unavailable if stopped

That could be tricky to do - after all, you could just do foo=NRF.connect;NRF.stop();foo()

if we also had resizable variables

That would be cool - also a complete can of worms. You'd have to move everything to lower var numbers in order to turn BLE on again :s

@fanoush
Copy link
Contributor Author

fanoush commented Apr 21, 2020

Wow, that's impressive! What is it you're doing while BLE is off?

espruino is quite flexible so I use it as first thing to flash to new smart watch, then after backing up bootloader I need stuff like

  • clear bootloader completely (so softdevice jumps directly to app after reset), then flash new one to possibly different location - both actions need erasing/writing UICR registers which is not possible when softdevice is on. E.g. in P8 watch there is custom bootloader but the UICR bootloader settings are not set which breaks when overwriting with nordic dfu bootloader. nordic one needs both 0x14 and 0x18 uicr registers to be set.

  • do buttonless dfu and enter bootloader. With ds-d6 you can just poke 1 to GPREGRET and it gets written and reboots but the value is there and bootloader jumps to DFU mode, however other nordic based bootloaders expect different value in GPREGRET but also expect softdevice to be left enabled by app to continue with buttonless dfu, so the easiest is to poke the value there during NRF.restart (or there is sd_power_gpregret_get/set/clr api that would need to be exported)

So basically NRF.onRestart hook is good enough for everything mentioned above.

I'm really starting to run low in available flash now, so I'm not sure it'd be worth removing other features to get this one working.

Yes, sure. maybe few if-s at runtime won't add much or even anything over current state.
Anyway, I need to work more with own fork and branches so I can keep such changes for longer and possibly show you the result. now I have these as random changes on top of your tree and just stash and apply them. not very flexible

That would be cool - also a complete can of worms. You'd have to move everything to lower var numbers in order to turn BLE on again :s

Well, didn't look how good is garbage collection and defragmentation but I hoped it would work. And even now in banglejs you do reset() between apps - clearing variables, so it could work like this, you could restart to resize it but you would not need to build different firmware to change MTU, or possibly also resize stack. Imagine you need more variables but don't write deeply nested code then you could change the ratio, or if your code fails with stack overflow you could make stack larger after next reboot. So it would be pretty static at runtime, with no extra runtime code, just different sizes chosen at boot time.

And turning off softdevice could also work in this way fact - reset() or E.reboot() with flags to set variables, stack, mtu sizes and possibly also disable softdevice. Then the NRF object could be set up in different way at startup (possibly use only some subarray of method pointers).

Well but yes, it can make stuff larger and nrf52832 is now becoming too small indeed. However with such fine tunings the life of 52832 could be extended a bit. E.g. on banglejs with bluetooth turned off in settings (-> hidden restart + softdevice disabled) one could run larger app/game when not using watch paired to anything.

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.

None yet

2 participants