Freezing watch when uploading a watch face with 15+ properties

[vaspa]

I've tested my weather plugin with ~50 properties for weather forecast and noticed that the watch freezes often or does not show plugin properties. Checking experimentally the number of properties when it is still working correctly I found that the limit is around 15 properties. Furthermore, it is not precise, once a watch face with 17 props uploads correctly, second time the same one freezes the watch. At the same time the watch face preview in Android emulator is OK (even with ~35 parameters). First intuition: it is a transfer problem or a RAM problem when compiling JSON.

[althink]

There's not much RAM left for watchsets, in latest version it's just 512 bytes to allocate. Two kind of data are kept there: current value of external properties used in given watchset and last value rendered by every control on current screen (4 bytes per number control and max text length+1 for text controls). So the issue may be missing ram or some bug. There are two ways to fix this:

1. Keeping all watchset data in external ram (didn't manage to do that in 0.4.0) - there are around 3kB left
2. Keep only external properties data for given screen but then there will be some (1sec) delay between screen change and the moment when data will appear.

Please attach some sample watchsets so I will debug and check the reason.

[vaspa]

Well, after playing with all my freezing watchsets I can not upload them anymore. The error "Data upload failed". I tried resetting the watch, rebooting the phone and even made a DFU with the latest release. It seems that the last watchset is still there even after the DFU. If I press "UP" the watch hangs. And without uploading I cannot overwrite the existing watchset. Is there a way to "clean" it?

[althink]

Try this version, I've added "Format" option to the Settings screen. Format will take around 5 seconds.

https://www.dropbox.com/s/zmxqax2yxbft9ba/ossw-firmware-s120-snapshot.zip?dl=0

[vaspa]

Great, the "format" revived the watch! It seems that the file system was corrupted during the hard resets. I've added my "heavy" example to the samples.

[althink]

The problem is that this watchset uses too much ram. It has:

• 30 text controls
• 5 image set controls

So the required memory is:

• external properties values buffer requires: 30 * (32+1) + 5 * 1 + 35 * 4 = 1135 bytes
• current screen buffer requires: 5 * 4 + 30 * (32+1) = 1010 bytes

It's 2145 bytes total and we have 512 bytes available ;)

There are many places to improve:

• external properties values buffer
  • 35 * 4 - this part (properties index and value offset) may be moved from ram to compiled watchset data (flash) the same way as I did this in current screen buffer.
  • 30 * (32+1) - text properties buffer - we should add possibility to define max text filed length in watchset definition so we will reduce ram requirements. For this watchset it could be: (6+1) * 15 for temperature vales, (4+1) * 5 for humidity, (5+1) * 5 for wind speed and (7+1) * 5 for pressure. It's 200 bytes total.
• current screen buffer
  • 5 * 4 - it should be reduced to 5 *1 because values are 1 byte long (now every number control has 4 bytes cache buffer)
  • 30 * (32+1) - the same story as in external properties values buffer.

With those fixes we should be able to fit into current memory limit. But we can go a bit further and reduce ram size even more:

• Do not use text properties, use number and enum type instead. It would require to add support for negative values but it's not rocket science ;) The same screen could be composed of:
  • 15 numbers 2 bytes each (temperature)
  • 1 byte temperature unit enum
  • 5 numbers 1 byte each (humidity)
  • 1 byte humidity unit enum
  • 5 numbers 2 bytes each (wind speed)
  • 1 byte wind speed unit enum
  • 5 numbers 2 bytes each (pressure)
  • 1 byte pressure unit enum 59 bytes total

So after this change this watchset would require 59 bytes for external properties values buffer and 95 bytes for current screen buffer = 154 bytes total. Current screen buffer is bigger because every "unit" control would have it's own 1 byte "last rendered value" buffer. This buffer allows not to redraw given control if value has not changed from the last render.

[vaspa]

Detailed analysis ;) I had a feeling that strings take too much memory. I did not dig the code yet, I shall install the nRF51 SDK this week-end and start playing with it (unfortunately, I'm not an expert in embedded systems). I agree completely that I abused strings due to the lack of negative numbers and fixed length string constants (e.g. the temperature could be represented as a float concatenated with "°С" constant which can be shared with all other temperature controls). But maybe I'm also missing a few points:

• Do you use fixed length strings to cope with RAM fragmentation?
• Where are the coordinates/styles for each screen control stored? Compiled on flash storage?
• Are the external properties that are not used in currents watchface also updated regularly?
• Why do we need the external properties values in the RAM? I agree that they could be referenced and stored on flash (some of them could be even volatile and should be re-fetched from the external sensor/phone).
• I suppose that the images are not stored directly in the screen buffer. So, maybe the string properties from the current screen could be also stored on flash and references from the screen buffer.

Indeed if we specify the length of strings in JSON it can be directly a quick fix for the actual problem without all the planned optimizations required. It will have a size of ~ 5 * (6 strings of 7 bytes) = 210 bytes for the external properties. and/or Support for negative numbers (needed anyway) + optional attribute "unit" with some short constant, like: "mph", "bpm", "m/s", "hPa", "°C" stored with compiled data (btw, the degree sign is not rendered ;)

In your last example you didn't mention the enum/pointer for images... So, for the properties values we can even get: 5 * (image enum:1b + 3*temp:1b + humidity:1b + wind:1b + pressure:2b) + weather units:4b = 44 bytes

[althink]

Do you use fixed length strings to cope with RAM fragmentation?

When watchset is run memory buffer is allocated for all external properties that are in that watchset (but doesn't matter on which screen). For variable length properties like texts max allowed data size is allocated.

Where are the coordinates/styles for each screen control stored? Compiled on flash storage?

Until today they where in external flash and I've read them when data was rendered. Yesterday it turned out that spiffs is very slow when small data part is read many times. So today I've rewritten that part and all controls from current screen are copied to external ram on screen entry. Now it takes 25 times less time.

Are the external properties that are not used in currents watchface also updated regularly? Only properties that are on a given watchset are updated. When watchset is started it sends an id to the phone and phone updates external properties that are on that watchset (on any screen in that watchset)

Why do we need the external properties values in the RAM? I agree that they could be referenced and stored on flash (some of them could be even volatile and should be re-fetched from the external sensor/phone).

Storing fast changing data like heartrate, speed etc on a flash is a bad idea, it will kill it quite fast. Those are the most frequently read data so the faster we can read them the less power we consume, that's why RAM looks like a good idea (or external RAM if we'll be out of processor RAM :/ )

I suppose that the images are not stored directly in the screen buffer. So, maybe the string properties from the current screen could be also stored on flash and references from the screen buffer.

Static texts or dictionaries like "1 -> Sunday" will be there but properties that can change should be in internal/external RAM. Keep in mind that when you want to change 1 byte on a flash and it was already used and some bit that have to be set to 1 was set to 0 then you have to clear 4k bytes to be able to write that one byte. (and for sure you have to make a copy of 4k-1 bytes and write them one more time :) )

[althink]

I've aded some checks in code that prevents running watchset that tries to allocate more memory that is available. That's why your weather watchset will not start in 0.4.1 but I've fixed now few small bugs and increased a little bit stack and heap size and it should work fine in latest snapshot. And now it's quite stable.

[vaspa]

I've updated the weather-forecast.json sample file to reduce the lengths for each text field (with the corresponding fix in Android app). Strangely it worked in 0.4.0 and stopped working in 0.4.1. The watch hangs silently with the latter one. There are 5 blocks each one with 1 image and 40 bytes of strings, that is 220 bytes for external properties. Doubling the amount of memory should not exceed 580 bytes.

[althink]

I've used your latest watchset with length param with 0.4.0 and yes, it worked. The problem is that in both 0.4.0 and 0.4.1 there is a 512 byte limit for heap, and your watchset allocated around 570 without problem :) So I've debugged and fixed all places when malloc failed to allocate memory but I didn't checked that it failed and app used 0 as a pointer and used memory dedicated to softdevice :) When I've fixed that I've noticed that assigning even 700 bytes did not work. Then I've found places where arrays were defined like:

size = something calculated based on function argument uint8_t array[size];

I'm not a C developer (8 years in java world ;) ) so didn't know that this one use malloc to allocate memory. So I've fixed those places so only arrays on stack are used.

And last issue was that when you exceed stack limit there's no hard fault or something like that and stack just overrides heap... so I've tried to free some more memory to be sure that stack size is ok. And in latest version (not 0.4.1 but current state in git) there is 580 bytes for heap and your watchset works fine without stealing softdevice memory ;)

So summarizing:

• it worked in 0.4.0 because of a bug and fact that it used memory dedicated to softdevice
• it does not work in 0.4.1 because 512 bytes limit is respected (watchset should just not start but I think that there was a missing break statement so watch may hang)
• it works fine in latest snapshot because now there is 580 bytes limit for heap and is respected

[althink]

Here's latest built snapshot:

https://www.dropbox.com/s/1pgmqvhhml2nff2/ossw-firmware-s120-0.5.0-snapshot.zip?dl=0

[vaspa]

Thank you for the latest snapshot. Regarding the memory allocation: uint8_t array[size] is a VLA (variable length array) and normally should not be allocated on heap, therefore, should not be free'd afterwards.