lancaster-university / microbit-dal

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

MicroBitCompass stores its compassCal data to flash at every reset #140

Closed martinwork closed 8 years ago

martinwork commented 8 years ago

MicroBitCompass::init calls storage->get to retrieve the compassCal data then calls setCalibration.

setCalibration calls storage->put, which writes the same compassCal data back to flash.

finneyj commented 8 years ago

Thanks @martinwork

The function call to save the key-value pair should only prompt a physical write to memory if the value is different to that already stored... but i'm not seeing code to do that. Looks like a missing validation case in MicroBitStorage:put(). Good catch!

finneyj commented 8 years ago

Looking at this, there's a fair bit of overlap with the low level read/write calls used in MicroBitFileSystem. Aligning this would reduce necessary code and solve the problem in one go... James is going to pick this up.

jamesadevine commented 8 years ago

Will fix when #139 is merged.

finneyj commented 8 years ago

thanks

martinwork commented 8 years ago

Thanks @finneyj and @jamesadevine ... I noticed it because I had added some serial trace to flashPageErase. I didn't realise this was supposed to depend on the behaviour of MicroBitStorage. I was thinking of maybe replacing the lines in init featuring storedSample with memcpy( &average, calibrationData->value, sizeof(CompassSample)); status |= MICROBIT_COMPASS_STATUS_CALIBRATED;

BTW, am I right in thinking that saving the calibration can't be done while BLE is connected? It seems that if I try to calibrate with BLE connected I end up with a triangle of LEDs lit, apparently in flashPageErase.

finneyj commented 8 years ago

@martinwork yep - that'll do as a workaround.

yes, we had also occasionally noticed that feature with BLE connected - pretty sure it's when SoftDevice's high priority interrupt fires during an erase cycle. It's on the list... :-)

If you have a repro case though, perhaps you could try disabling interrupts during flashPageErase(), and report back if that solves the behaviour?

martinwork commented 8 years ago

Tried adding disable/enable_irq - no luck. The following seems to be about this situation: https://devzone.nordicsemi.com/question/1183/nrf51822-flash-erase-error/

finneyj commented 8 years ago

Many thanks - that does seem to reinforce our thoughts about this being the cause...

Surprising that disabling IRQs doesn't prevent this issue at the expense of introducing latency in the BLE stack. I'm guessing the BLE radio must be running at really high priority, and is getting masked by _disable_irq(), or the delay introduces an unresolvable fault in SoftDevice...

Thanks also for the link - this sounds like an approach we should explore...

p.s. can you confirm that the fault only arises when BLE is actually connected?

mmoskal commented 8 years ago

I was getting lockups when doing neopixel - which was only 1-2ms with interrupts disabled. Disabling BLE fixes the problem. They lockup would only happen after a few seconds, randomly.

finneyj commented 8 years ago

Thanks @mmoskal - yet more evidence of this.

martinwork commented 8 years ago

I haven't had a problem without BLE connected. I tried lots of variations on how and when to call calibration before I remembered that saving to flash and BLE didn't go together ( maybe should be mentioned in the docs?).

The link's idea of using a slow connection seems likely to fail occasionally.

I suppose maybe saving could be delayed until disconnection.

I have arranged to close the connection to calibrate. If saving were moved out of the calibration routine, I could message the central to call me back in a few seconds before I disconnected to save.

finneyj commented 8 years ago

we could do a delayed save, but this actually brings its own problems... e.g. it's necessary to persistently save some connection state to be BLE compliant (the CCCD state) - which in itself requires a flash erase/write. Hence we could actually wait pretty much forever for a disconnection... which may result in us never saving crucial state.

finneyj commented 8 years ago

although... I guess the calibration data isn't critical. :-)

mmoskal commented 8 years ago

I was getting lockups when no BLE device was connected. Just enabling BLE in compilation was causing trouble.

jamesadevine commented 8 years ago

@mmoskal I assume this was before the recent changes to the storage class...

When using locally scoped variables allocated on the stack, memcpying 48bytes, a lock up would occur consistently.

@martinwork what about you? Are you tagging against master or a specific version?

jamesadevine commented 8 years ago

Sorry, on my phone.

memcpying when the data size was less than 48bytes, and declared on the stack.

martinwork commented 8 years ago

I'm working with a local copy of "microbit-samples-master" with my own main(). The yotta_modules folder is identical to the current microbit-samples-master ( after a yotta build), except...

finneyj commented 8 years ago

Thanks @martinwork. We did put a new ticker implementation in there - perhaps we don't tear down the interrupt handler properly before reset...

mmoskal commented 8 years ago

@jamesadevine sorry, I should have been more explicit: I was getting lockups with neopixel (no compass) with no BLE connection.

jamesadevine commented 8 years ago

Thanks @martinwork for raising this, I've temporarily added a patch and it is in the latest tagged release, v2.0.0-rc3.

jamesadevine commented 8 years ago

@martinwork I'm going to close this issue, as this is resolved as of v2.0.0-rc3

Also, if you are interested in a DFU fix, pull the latest microbit-dal master, and update mbed-classic:

https://github.com/lancaster-university/mbed-classic/commit/1fb8ab4c1942d7eaa265e032e203c01a3bd71019

jamesadevine commented 8 years ago

P.S. it was actually a combination of this line and the underlying timer implementation that was causing the DFU bug, good find @martinwork !

martinwork commented 8 years ago

@jamesadevine thanks and well done

jamesadevine commented 8 years ago

:smile: along with @finneyj