AsteroidOS / asteroid-btsyncd

Bluetooth Low Energy synchronization daemon for AsteroidOS.
GNU General Public License v3.0
10 stars 17 forks source link

QByteArray is assumed unsigned in many places #26

Closed Noodlez1232 closed 1 year ago

Noodlez1232 commented 1 year ago

I found this through a different bug, but I have figured out that this bug is much more than the one bug. I accidentally was sending an invalid year in trying to implement AsteroidOS support for Gadgetbridge, which led to me sending a technically negative number (> 127). When this is added to 1900 in the year for TimeService, this is subtracted instead, giving a completely invalid year, which crashes the daemon.

QByteArray is apparently a fancy wrapper around const char*, but not const unsigned char*, so many values are incorrectly cast with signedness in mind.

This is done in timeservice.cpp:31, many times in weatherservice.cpp (mainly in the temperature stuff), in mediaservice.cpp:171 and a few others.

I can start writing a fix, but I have no clue how to upload and test this daemon on my watch/emulator, so some guidance there would be appreciated.

FlorentRevest commented 1 year ago

I found this through a different bug, but I have figured out that this bug is much more than the one bug. I accidentally was sending an invalid year in trying to implement AsteroidOS support for Gadgetbridge, which led to me sending a technically negative number (> 127). When this is added to 1900 in the year for TimeService, this is subtracted instead, giving a completely invalid year, which crashes the daemon.

QByteArray is apparently a fancy wrapper around const char*, but not const unsigned char*, so many values are incorrectly cast with signedness in mind.

This is done in timeservice.cpp:31, many times in weatherservice.cpp (mainly in the temperature stuff), in mediaservice.cpp:171 and a few others.

Thanks for looking into this @Noodlez1232 ! I'm sure there are plenty of bugs like this in asteroid-btsyncd and fixes are definitely welcome.

Also keep us updated on the GadgetBridge support, this definitely sounds exciting :) Maintaining AsteroidOSSync has always been a pain point for us and if GadgetBridge ends up being a more capable alternative at one point, I think I would consider abandoning AsteroidOSSync.

I can start writing a fix, but I have no clue how to upload and test this daemon on my watch/emulator, so some guidance there would be appreciated.

It's your lucky day, https://github.com/AsteroidOS/meta-asteroid/pull/125 just got merged so you can now generate SDKs that can build asteroid-btsyncd. If you need more help, maybe @dv1 can chime in with additional details

Noodlez1232 commented 1 year ago

Also keep us updated on the GadgetBridge support, this definitely sounds exciting :) Maintaining AsteroidOSSync has always been a pain point for us and if GadgetBridge ends up being a more capable alternative at one point, I think I would consider abandoning AsteroidOSSync.

There is a pull request right now with the support. With this support added, it will have complete feature parity with AsteroidOSSync, so it's pretty much done.

It's your lucky day, https://github.com/AsteroidOS/meta-asteroid/pull/125 just got merged so you can now generate SDKs that can build asteroid-btsyncd. If you need more help, maybe @dv1 can chime in with additional details.

That would be greatly appreciated! I'm still willing to do this, since the AsteroidOS watch is a daily-driver for me (replaced the PineTime), so any improvements are just scratching an itch for myself anyways.

dv1 commented 1 year ago

@Noodlez1232 You set up the SDK just like any other Yocto SDK. That is, follow the steps here. There are just two, really - the SDK installation, and later, sourcing the env script to set up the environment. Once that is done, with the changes I pushed, the SDK will be set up with proper support for CMake, qmake, and QtQuick / QML.

Then go to the project you want to build and build it as usual. For example, with asteroid-btsyncd, you first source the env script (I built my SDK for tetra, aka the Sony Smartwatch 3):

source /home/test/yocto-sdk/asteroidos-tetra/environment-setup-armv7vehf-neon-oe-linux-gnueabi

Then you build asteroid-btsyncd with the standard CMake calls:

mkdir build && cd build
cmake ..
make -j4

Copy over binaries with SSH (see here for how to do that with AsteroidOS). To do that, the active asteroid-btsyncd session needs to be stopped. open a second terminal (the first one is where I built asteroid-btsyncd) and log into the smartwatch with SSH. There, I stop the active session:

systemctl --user stop asteroid-btsyncd

Then you can copy over your newly built copy with scp (SSH copy) while still being in the build folder:

scp src/asteroid-btsyncd root@192.168.2.15:/usr/bin/

asteroid-btsyncd resides in /usr/bin/ on the smartwatch. Writing to /usr/bin/ requires root permissions, which is why I don't use ceres as the user here. Also, you might want to use a different IP address if necessary, though 192.168.2.15 usually works for me.

Then you can start it again:

systemctl --user start asteroid-btsyncd

That's it. It is pretty much the same with asteroid-launcher, except that this one uses qmake instead. Also, restarting asteroid-launcher like this does not seem to work fully - I can't access menus like Settings with this, and have to restart the entire smartwatch instead.

I also recommend opening another terminal, logging in via SSH as root, and starting journalctl -f. That way, you see all the log output on the system, including asteroid-btsyncd and asteroid-launcher.

dv1 commented 1 year ago

@Noodlez1232 BTW - I have been working on a way to forward data from other apps through AsteroidOSSSync to watchfaces. That way, there can be watchfaces that show extra data that is specific to these apps. See my AsteroidOSSSync fork and my asteroid-btsyncd fork. Do you think that this simple external app message feature could be added to GateBridge?

Noodlez1232 commented 1 year ago

@Noodlez1232 BTW - I have been working on a way to forward data from other apps through AsteroidOSSSync to watchfaces. That way, there can be watchfaces that show extra data that is specific to these apps. See my AsteroidOSSSync fork and my asteroid-btsyncd fork. Do you think that this simple external app message feature could be added to GateBridge?

@dv1 Unfortunately not yet. It could be added, however it's important to remember that GadgetBridge is supposed to be a device agnostic codebase, and super special support for a singular watch's capabilities is generally not wanted. The correct way to do this is to actually communicate with the GadgetBridge team on a case-by-case basis for each application and how to communicate in a standardized way between not just this watch but all watches. E.g. adding the ability to send maybe the keychain unlock status from OpenKeychain. You'd need to work with the GadgetBridge on creating an intent for signaling maybe org.gadgetbridge.keychain_lock_status. This way it won't be just this watch that can get that information, but any watch that can get that information and display it in a way that fits the watch's communication methods.

I think another way is to add to GadgetBridge a "Custom information intent" where you can send watch-specific information across into GadgetBridge for the device to work with and dispatch as needed. Maybe it can take a package name as the source, a subject, and a body, much like you have being sent, but this would add some extra complexity to other watches as well.

That being said, GadgetBridge's codebase is super easy to work with, and I think taking a look at their application and seeing how they do these types of things may be helpful. I think that GadgetBridge is the way to go for AsteroidOS from now on as well, since constantly maintaining AsteroidOSSync seems to be a pain in the butt.