Skip to content

Conversation

@FintasticMan
Copy link
Member

The message array in the Notification struct has a size of MaximumMessageSize + 1, for a trailing null terminator if the message is MaximumMessageSize chars. This extra char isn't used for notifications coming from bluetooth, meaning that notifications are truncated 1 character earlier if they are too long. This change is minor and just means that notifications can be 1 character longer.

@FintasticMan FintasticMan added the maintenance Background work label Mar 17, 2023
@FintasticMan FintasticMan requested a review from a team March 17, 2023 13:26
@github-actions
Copy link

Build size and comparison to main:

Section Size Difference
text 406604B -32B
data 940B 0B
bss 53568B 0B

Copy link
Member Author

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution to this would be to not make the message array 1 char bigger, which would make it more obvious when a size/len does or doesn't include the null terminator.

@Riksu9000
Copy link
Contributor

I think the terminator will always cause confusion as long as the truncation needs to happen outside of NotificationManager. I think NotificationManager should accept any std::string and truncate it internally.

@FintasticMan
Copy link
Member Author

I think that would be a shame personally, because I think it should be possible to make it clear what is what through good naming and consistency. For example by using len to indicate number of chars excluding null terminator, and size being number of bytes total.

Anyway, the reason I would rather not use std::string is because it is always allocated on the heap, which seems wasteful if we're just putting it onto the stack again right after.

@Riksu9000
Copy link
Contributor

Would string_view be better?

mark9064 added a commit to mark9064/InfiniTime that referenced this pull request May 8, 2023
…niTimeOrg#1695

commit d7ca78b
Author: Finlay Davidson <finlay.davidson@coderclass.nl>
Date:   Fri Mar 17 13:48:26 2023 +0100

    alertnotificationservice: Make use of extra char in message array for \0

    Also move category reading down for clarity

commit d1b16a7
Author: Finlay Davidson <finlay.davidson@coderclass.nl>
Date:   Fri Mar 17 13:38:52 2023 +0100

    alertnotificationclient: Make use of extra char in message array for \0
mark9064 added a commit to mark9064/InfiniTime that referenced this pull request May 27, 2023
…niTimeOrg#1695

commit d7ca78b
Author: Finlay Davidson <finlay.davidson@coderclass.nl>
Date:   Fri Mar 17 13:48:26 2023 +0100

    alertnotificationservice: Make use of extra char in message array for \0

    Also move category reading down for clarity

commit d1b16a7
Author: Finlay Davidson <finlay.davidson@coderclass.nl>
Date:   Fri Mar 17 13:38:52 2023 +0100

    alertnotificationclient: Make use of extra char in message array for \0
@FintasticMan FintasticMan marked this pull request as draft June 17, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Background work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants