espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.75k stars 741 forks source link

JS variables used before initialised when BLE init #1696

Closed gfwilliams closed 11 months ago

gfwilliams commented 4 years ago

Looks like this: https://github.com/espruino/Espruino/blob/6a4758160c4aa3c95a60569ec5ea9f57f8e41df5/targets/nrf5x/bluetooth.c#L2408

http://forum.espruino.com/comments/14925743/

fanoush commented 4 years ago

So moving jsvInit before jshInit (variables before hardware) is conceptually wrong? THe ble code may try to read some default for mac address from variables which may look useful in general. So if jsvInit could also handle some pre-population of default values of some variables from some static data the ble_init code could make sense then. Why not moving jsvInit before jshInit in general? Can this break something else?

gfwilliams commented 4 years ago

moving jsvInit before jshInit (variables before hardware) is conceptually wrong?

The idea is that jshInit should really be low-level hardware. For example in systems that support it (stm32 or nrf52840) you might initialise QSPI RAM, and then put the variables in there.

Ideally BLE wouldn't be initialised there, but it's probably needed for other stuff. Maybe not though? Maybe it could be initialised later on.

if jsvInit could also handle some pre-population

That's what jsiInit does - which has to come after jshInit.

fanoush commented 4 years ago

I see. So looks like the HW stuff could be splitted to really low level basic global stuff (ram, stack, cache, cpu) and then the higher level. Because even e.g. the uart initialization and then console setup in jshInit is pretty high level and could setup Serial1 variable properly with speed etc

fanoush commented 4 years ago

The idea is that jshInit should really be low-level hardware. ... you might initialise QSPI RAM, and then put the variables in there.

When looking at nrf5x jshInit I am really not sure if there is anything at all which is so low level on this platform as there is no such stuff like setting stack, cpu clock or cache, ram timings, enabling FPU etc.

gfwilliams commented 4 years ago

Ahh - when I push the smartwatch stuff it'll initialise an external flash chip in there at least, which will then be used for loading saved code into RAM.

fanoush commented 4 years ago

Ahh - when I push the smartwatch stuff it'll initialise an external flash chip in there at least,

What I wanted to say that currently most of the code is in fact high level stuff that belongs after variable (=javascript memory) initialization. So I'd suggest to add jshLowInit and move only stuff that are needed for variable init there and keep rest in current jshInit and move it after jsvInit and use javascript variables in jshInit freely (like e.g. setting Serial1._options properly when initializing it and using serial console). That also looks better to me than moving BLE init elsewhere into some new jshHighInit.

gfwilliams commented 4 years ago

Just had another look into this - everything I can find called from jsble_init is using jsvObjectGetChild(execInfo.hiddenRoot, ... and execInfo.hiddenRoot should be zero at boot time (all RAM should start off 0?) and whenever the variable list gets torn down, so the very first check of jsvObjectGetChild would cause 0 to be returned, which would be fine.

Have you seen this problem since @fanoush?

I get your point about having two initialisation functions, but I'd argue that some of jsble_init (like setting up the low frequency oscillator) was low level enough that it should happen right at the start. I guess that could be split in two as well, but it's a big undertaking to do right.

Perhaps the better option is to do the super low level stuff in jshInit (like the clock) but to initialise the BLE stack in jswrap_ble_init which is called at startup anyway.

gfwilliams commented 11 months ago

I'm pretty sure this can be closed now - I had a go through this a while back to make sure all the uses during bluetooth init were safe.