Lite board fails to boot if too many totp codes are configured.

[madhogs]

If, on the totp_face, I add more than just a few totp codes the watch fails to boot and doesn't display anything. I think this is to do with the new credentials configuration as I have the same credentials working on my other (also sensor watch lite) watch which is still on the old firmware.

I have created a branch here - https://github.com/joeycastillo/Sensor-Watch/compare/main...madhogs:Sensor-Watch:example-too-many-totp The above has some example credentials that are the same length and number as the real ones i have. After building the above with 'make color=RED' and flashing the watch it no longer displays anything when the battery is connected. Reducing the list to 5 or less entries and it works again.

It always works when running in the emulator, it only fails on the physical watch, i wonder if it is some hardware limitation when converting these codes.

[matheusmoreira]

I tested this on my watch with 5 entries. I suppose I was just under the limit. :(

Could it be running out of memory? In order to achieve that CREDENTIAL interface, I made the watch decode the base 32 strings at runtime. The TOTP LFS face works like that too so I thought it was OK. Now I'm assuming it's going to run into the same limitations you described. Looks like the impact on RAM usage was higher than expected...

There are some improvements I can make to both faces to immediately reduce memory usage: decoding the string every time the face activates instead of decoding them all at once during initialization. Can you help me test these fixes and confirm whether it resolves the issue?

The other alternative is to go back to storing the raw decoded byte arrays in the source code which in my opinion made the face very hard to use since web sites don't output that format. @maxz has submitted a Python script to generate those arrays from a separate file at compile time, we should probably make that a priority.

[madhogs]

Not sure really how to even work out what is failing on the physical watch as the emulator works fine. I was also unable to get the lfs face to work on the physical watch too, could be related? In that case it would load the first two but the rest would not appear (possible i've done something wrong there though).

More than happy to test your changes if you have some ideas already on how to reduce the memory usage.

I definitely prefer not having to input the raw decodes bytes, but obviously if that works better/allow more credentials than happy to go back to that method.

[matheusmoreira]

More than happy to test your changes if you have some ideas already on how to reduce the memory usage.

Thanks!! I'm developing a hot patch right now, I'll post the branch here once it's compiling.

I was also unable to get the lfs face to work on the physical watch too, could be related?

Yes. I used the same algorithm as the TOTP LFS face. That made it require more RAM.

In that case it would load the first two but the rest would not appear (possible i've done something wrong there though).

That's weird. I expected the limitations to be around the same but TOTP LFS supports even less codes? I didn't touch the TOTP LFS face in the latest commits either, so I conclude this problem must have been there for a while now.

If the hot patch fixes the issue for you, I'll apply it to the LFS version as well.

I definitely prefer not having to input the raw decodes bytes, but obviously if that works better/allow more credentials than happy to go back to that method.

Wish C had better compile time code generation support. Macros are extremely crude tools. Something like Zig's comptime would completely solve this.

Guess we'll have to make do with a Python script.

Not sure really how to even work out what is failing on the physical watch as the emulator works fine.

QEMU would have been really useful right now. I need to figure out how to port the Sensor Watch board to it.

[matheusmoreira]

Okay, I developed a hot patch which refactors the face's memory allocation.

https://github.com/matheusmoreira/sensor-watch/tree/totp-hot-patch

Please verify if it solves the problem. I'll submit a pull request immediately if it does.

[madhogs]

That branch seems to fix the issue :+1: , was able to load the example codes from the above branch no problem.

I've only done a quick test as its quite late here, only checked it loaded, happy to test it more + check the values tomorrow.

Thanks for looking into this so quickly.

[matheusmoreira]

Great! Thanks for testing!

[maxz]

Oh boy. Unless you find another solution to the possible repeated runtime allocation (on a quick glance it at least does not seem to call malloc for the same entry twice but to use memset) and decoding I guess I would prefer for the solution in the main branch to be the one from my PR #356 which has to deal with the explicit key array but has very little runtime overhead. I will at least maintain my own patch for my watch if this behaviour remains.

@joeycastillo also spoke out against runtime heap allocation within faces in the past (But I can't find the PR right now, it was some PR by @theAlexes so they might remember which one it was.) In that particular case I think he accepted the allocation in the face's initialisier.

[matheusmoreira]

@maxz I have eliminated the dynamic memory allocation. Now there is a single user overridable 128 byte buffer. Only the runtime decoding overhead remains which will also be eliminated when the Python script is integrated.

Frankly I'd prefer a more complicated setup over increased battery usage.

Me too... But we are programmers. Users might not be comfortable with munging bytes and editing them into source code. The watch face's documentation also suggests using a web service to decode the bytes which could leak the secret. I really wanted to avoid the need for that.

@joeycastillo also spoke out against runtime heap allocation within faces In that particular case I think he accepted the allocation in the face's initialisier.

Yes, in the discord:

fwiw I think it’s a best practice to avoid dynamic memory allocation outside of the watch face setup function. As it stands now (at least in the case where the watch is on-wrist and not plugged into USB) memory is only allocated once, at boot, and that doesn’t change until the battery dies. this feels good for the stability of the thing when it has to run for, y’know, two years

I agree with him and have therefore removed the dynamic memory allocation.