FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Refactor NotificationManager::Notification by DavisNT · Pull Request #2347 · InfiniTimeOrg/InfiniTime · GitHub

/ InfiniTime Public

Refactor NotificationManager::Notification - #2347

Open
DavisNT wants to merge 1 commit into
InfiniTimeOrg:mainfrom
DavisNT:notifrefactor
ConversationCommits (1)ChecksFiles changed
Open

Refactor NotificationManager::Notification#2347
DavisNT wants to merge 1 commit into
InfiniTimeOrg:mainfrom
DavisNT:notifrefactor

Conversation

DavisNT commented Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Refactor NotificationManager::Notification to use constructors. This reduces risk of coding errors (incl. buffer overflows) when creating NotificationManager::Notification and copying text to it.

Additionally fix a latent bug in ImmediateAlertService (include null terminator in the bytes copied and properly set size) using the constructor.

This PR supersedes #2159

This PR needs corresponding refactoring in InfiniSim: InfiniTimeOrg/InfiniSim#181

DavisNT mentioned this pull request Oct 2, 2025
Open

github-actions Bot commented Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown

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

DavisNT mentioned this pull request Oct 2, 2025
Merged

DavisNT commented Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

@mark9064, @NeroBurner Here is refactoring of NotificationManager::Notification with constructor. Can you please review this?

P.S. InfiniSim doesn't build, because it needs InfiniTimeOrg/InfiniSim#181

mark9064 added the maintenance Background work label Oct 2, 2025
NeroBurner added this to the 1.17.0 milestone Nov 4, 2025
BurninTurtles mentioned this pull request May 25, 2026
Open
DavisNT force-pushed the notifrefactor branch 2 times, most recently from 181276e to 37a9a18 Compare July 20, 2026 20:33

DavisNT commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@mark9064 , @NeroBurner I have rebased this PR (and retested services).
Could it be possible to do the final review/verifications and merge this PR (and the corresponding InfiniSim PR InfiniTimeOrg/InfiniSim#181 )?

DavisNT commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@mark9064 , @NeroBurner Could it be possible to merge this PR (and the corresponding InfiniSim PR InfiniTimeOrg/InfiniSim#181 )?

mark9064 reviewed Aug 11, 2026
View reviewed changes

mark9064 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry it's been a while. Check the date on that first comment!

Comment thread src/components/ble/NotificationManager.cpp Outdated
Comment thread src/components/ble/NotificationManager.cpp
Comment thread src/components/ble/NotificationManager.cpp

DavisNT commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@mark9064 Sorry - I had somehow missed your comment about the cast style! Now it should be fixed in both places.
Docstrings are also added.
Can you please review?

mark9064 approved these changes Aug 13, 2026
View reviewed changes

mark9064 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Haha sorry - the comment on the cast wasn't published, I wrote it a long time ago but never completed the review for some reason :)

Comment thread src/components/ble/NotificationManager.cpp Outdated
* @param size the size of complete message, including the final 0x00 byte
*/
NotificationManager::Notification::Notification(const char* message, uint8_t size) {
uint8_t effectiveSize = std::min(std::max(size, static_cast<uint8_t>(1)), NotificationManager::MessageSize);

mark9064 Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm curious if this could be just 1U, not sure if that would work. Probably not

DavisNT Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unfortunately neither 1u nor 1U worked here. 🙁

NeroBurner Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

uniform initialization could work and is a bit shorter uint_8t{1}. It is a form I'm not so used to use, and I don't know if it is "good" C++ 😅

mark9064 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I just had a look at this at this might be the best way - the argument supposedly is with uniform initialisation it's impossible to initialise with an invalid value, whereas this is possible with a cast

DavisNT Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, @NeroBurner!
I have updated the PR to use uniform initialization and rebased.

P.S. @mark9064 Could the build-firmware be failing due to recent GCC and linking changes?

mark9064 Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It is - I'm fixing it soon

DavisNT force-pushed the notifrefactor branch 2 times, most recently from 1a38d3c to 1eef235 Compare August 17, 2026 22:51
Refactor NotificationManager::Notification
a01e6eb 
Refactor NotificationManager::Notification to use constructors.
This reduces risk of coding errors (incl. buffer overflows) when
creating NotificationManager::Notification and copying text to it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

NeroBurner NeroBurner left review comments
mark9064 mark9064 approved these changes

Assignees

No one assigned

Labels

maintenance Background work

Projects

None yet

Milestone

1.17.0

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL