search

toniebox-reverse-engineering / teddycloud

teddyCloud is an open source server replacement for the Boxine Cloud
https://toniebox-reverse-engineering.github.io/docs/tools/teddycloud/
GNU General Public License v2.0
456 stars 34 forks source link

On ARM TeddyCloud time stays 0 when not forwarding to cloud #106

Closed televator closed 6 months ago

televator commented 9 months ago

TeddyCloud time stays 0 when not forwarding requests to the Boxine cloud.

image

Setup: TeddyCloud running on Raspberry Pi Zero W (arm6l) within a container, latest alpine version.

I debugged the issue and the problem is on formatting the time_t osSprintf(response, "%" PRIuTIME, time(NULL));

The PRIuTIME that comes from cyclone common describes time_t as %lu, so as unsigned long. On ARM, however, time_t is an unsigned long long. I noticed that @SciLor explicitly silenced the warning for this, but I missing the whole picture. There are multiple stackoverflow threads on this topic on how to get the proper printf-symbol, the most helpful being this.

I suggest to cast the calls to time(NULL) to unsigned long long, so make them osSprintf(response, "%llu", (unsigned long long)time(NULL));. I can open a PR with this, if you agree.

SciLor commented 9 months ago

Thank you for digging into this. It was just a quick win.

I don't think this would fix the underlying problem. It looks wrong to me to fix it by an explicit cast. (As this needs to be done everywhere) The source is the wrong PRIuTIME. Explicitly setting this to ull on ARM (32 bit only?) would mitigate that.

televator commented 9 months ago

Thanks @SciLor. I digged a bit more into this, trying this on different architectures (amd64, armv6) with multiple derivatives (debian, raspberry pi os, alpine), couldn't reproduce any of this on godbolt (with different archs and compiler versions). The problem seems to be just to alpine on arm (64, 32bit not known). You might have seen other problematic combinations in your build pipelines before silencing the warning, but for now I assume this is limited my setup. That explains why cyclone common PRIuTIME is good enough.

There does not seem to be a nice solution. Docs show the recommendation is not to use the time_t value directly, but not viable alternative for a unix timestamp is given. I totally agree that the cast - despite being the safe and small hack - is considered too ugly.

SciLor commented 6 months ago

This should be addressed with the latest build.