Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

header size is 3 only for customHuami category #1968

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmlich
Copy link

@jmlich jmlich commented Jan 15, 2024

According to GATT spec the header should be just 2 bytes.

Gadgetbridge and Amazfish implements Alert Notification Service compatible with Huami amazfit devices and uses HuamiCustom category. This allows using one extra byte in header to specify extra icon for notification.

fixes: #1895

Copy link

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

@FintasticMan
Copy link
Member

FintasticMan commented Jan 15, 2024

We shouldn't be adding a CustomHuami category, as we are completely unrelated to Huami. What we should be doing, is just assuming the header size is 2, unless the 3rd byte is below 0x20. This should also be done in both AlertNotificationClient and AlertNotificationService, instead of just the service.

header size should be 2 according to specification

fixes: InfiniTimeOrg#1895
@jmlich
Copy link
Author

jmlich commented Jan 15, 2024

In my opinion, checking the 3rd byte against 0x20 is an ugly hack.

I prefer to follow the implementation of companion apps, which introduce their own category.

On the other hand, we can adhere to the standard. In that case, we should probably check if the category is within the expected range. By the way, the specification also states that the maximum length of the payload is 18 bytes.

@FintasticMan
Copy link
Member

Yes, I agree that it is an ugly hack, but I would say that it's only a temporary solution while companion apps transition to using a header size of 2. After that, we remove the check and we adhere to the standard (other than that the payload is a lot bigger than the official max).

@jmlich
Copy link
Author

jmlich commented Jan 15, 2024

I think the font may not include letters below 0x20 (not sure). The easiest solution is to simply allow rendering white space.

@FintasticMan
Copy link
Member

Values below 0x20 are control codes, so the have effects on the rest of the text. Because of this I'd say it makes most sense to filter them out.

@jmlich
Copy link
Author

jmlich commented Jan 17, 2024

honestly, I would prefer this solution because removal of non-standard huami category and extra byte will require more changes in Amazfish and the category isn't processed by infinitime anyway. It will probably lead to some ugly hack the as well.

@JF002
Copy link
Collaborator

JF002 commented Feb 11, 2024

It's funny how a single byte can generate such complex issue, isn't it? :D

I checked the GATT specification : the header should be 2 bytes long and the payload limited to 18 bytes which is... very restrictive! I can't see any reasonable way to implement modern notifications with such a limitation!

At InfiniTime, we try to follow the specifications of the standard service as best as we can, but I think we can agree that increasing this 18 bytes limit was a good idea.

It's a bummer that I implemented it wrong with this 3 bytes header though. But since it's already implemented that way for a few years and that all companion apps have already integrated it, I don't think that changing to a 2 bytes header would be a good idea : it would generate confusion and compatibility issues for next to no additional benefit.

Regarding the support of Huami category. I mostly agree with @FintasticMan that we shouldn't add anything that is related to another (proprietary?) product in InfiniTime.

Or don't we? I mean, adding a few icons to the notifications might be good for user experience. And, if we decide to implement this, it would be easier for everyone to just use a protocol that already exists and that is already implemented by companion apps.

But adding those icons does not seem to be the scope of this PR : this PR changes the header to 2 bytes except if the Huami category is detected. That looks like a good idea : this would allow us to be compliant to the spec while still supporting the "Huami" extensions.

However... won't the changes in this PR break support with virtually all companion apps that have implemented the 3-bytes header? Is this really something we want to do?

@jmlich
Copy link
Author

jmlich commented Feb 12, 2024

After merging of this patch we can remove (revert) PineTime specific workaround which allows 3 bytes header all the time.

piggz/harbour-amazfish#286

Without allowing Huami Category, we must adjust our implementation in order to use just standard categories, which doable. This mapping should be tested against all platforms (SailfishOS, Ubuntu Touch, ..). On the other hand Infinitime doesn't show category name or icon anyway.

Also GadgetBridge is using CustomHuami category to InfiniTime:
https://github.com/Freeyourgadget/Gadgetbridge/blob/275deb4d06de6b64c0d7e73d13f6f2ad2879fe25/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/pinetime/PineTimeJFSupport.java#L342
I am not able to say, if they have similar workaround to send 3 bytes header. I am not able to test that easily as I don't have Android on my phone.

@JF002
Copy link
Collaborator

JF002 commented Feb 15, 2024

After merging of this patch we can remove (revert) PineTime specific workaround which allows 3 bytes header all the time.

This would "couple" a specific version of InfiniTime with a specific version of all companion apps that currently implement the notification. They probably all implemented such workaround to use a 3 bytes header instead of a 2 bytes one.
And if we decide to switch to a 2 bytes header, all companion apps will have to remove that workaround.

Another consequence of this change is the retrocompatibility : older versions of InfiniTime won't work well with newer versions of companion apps and newer versions of InfiniTime won't work well with older versions of companion apps. This is something we have to avoid as much as possible.

@jmlich
Copy link
Author

jmlich commented Feb 16, 2024

I see, the backward compatibility with old InfiniTime is indeed trouble. The change will lead to missing first letter of the Caller Name. I guess, we will add some extra condition into Amazfish. The goal is to get rid of this hack in long term.

If you move to 2 bytes header only (without HuamiCategory), the retrocompatiblity issue will be a little more complex, because category assignment needs to be changed.

I guess, we should implement some version warning or update suggestion into Amazfish for such cases. However, some people may prefer to stay with older version of InfiniTime because they have fork with own patches.

@mark9064
Copy link
Contributor

mark9064 commented Feb 16, 2024

I think aligning with the spec is the most important long term. It's true that older versions of InfiniTime will lose one character. But this isn't a terrible incompatibility, and the solution is simple (upgrade). They'd also have to be using old InfiniTime with a new companion applications - setups where neither side is updated will remain unaffected. If someone is maintaining a fork based on an old version, they will be able to cherry pick this patch easily.

By deviating from the spec, we become incompatible with the potentially larger range of applications that implement the spec as is. Since we have an easy workaround of checking whether the 3rd byte is < 0x20, why not implement this and be compatible with both the spec and the 3-byte incorrect version?

@JF002
Copy link
Collaborator

JF002 commented Feb 25, 2024

Well we have some conflicting opinions here. This is fine, but I don't know which side I would chose.

  • The original PR wants to add support for Huami category by using/adding an extra byte in the notification header
  • Some comments say that we shouldn't implement feature that are specific to Huami devices
  • Some comments say that we should stick to the specifications and revert to a 2-bytes header
  • Some comments say that we should support both 2 and 3 bytes header, some do not agree

My opinion here is that changing anything will lead to compatibility issues with companion apps (which is something I would like to avoid) and extra work for many developers without bringing any new value to the user (at least on the short term). Yes, we are not compliant to the spec, but basically all companion app that support InfiniTime have already implemented it that way.

But I'm OK to talk about this. Maybe someone will come up with a plan that ensure that changes in InfiniTime and in all companion apps are synchronized so we do not break the user experience too much, for example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AlertNotification Service/Client are not compliant to the spec
4 participants