Add a global AppIdService to the application architecture#231
Open
J0EK3R wants to merge 11 commits into
Open
Conversation
J0EK3R
force-pushed
the
merge/vicocz/AppIdService
branch
from
77a6abc to
2d562db
Compare
added 5 commits
June 11, 2026 13:28
There was a problem hiding this comment.
Pull request overview
This PR introduces a single, app-global Application Identifier service that is persisted via PreferencesService, and wires CaDA, MouldKing, and JIESTAR to use this shared AppId source (instead of vendor-specific persistence/logic).
Changes:
- Added
IAppIdentifierService+AppIdentifierServicethat generates/persists a random app identifier and can serve variable-length prefixes. - Refactored CaDA and JIESTAR device managers to pull their AppID from the shared service.
- Updated MouldKing architecture so devices can receive an app-wide AppID (via
IMouldKingDeviceManager) and patch telegram bytes [1] and [2]; updated/added tests accordingly.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| BrickController2/BrickController2/UI/Services/AppIdentifier/IAppIdentifierService.cs | New interface for requesting an AppID prefix of a given length. |
| BrickController2/BrickController2/UI/Services/AppIdentifier/AppIdentifierService.cs | New global AppID implementation backed by IPreferencesService. |
| BrickController2/BrickController2/UI/DI/UiModule.cs | Registers AppIdentifierService as a singleton in UI DI. |
| BrickController2/BrickController2/DeviceManagement/CaDA/CaDADeviceManager.cs | Switches CaDA to use the shared AppID and removes vendor-specific persistence. |
| BrickController2/BrickController2/DeviceManagement/JieStar/JieStarDeviceManager.cs | Switches JIESTAR to use the shared AppID and removes vendor-specific persistence. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/IMouldKingDeviceManager.cs | New interface to expose the MouldKing AppID to devices. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MouldKingDeviceManager.cs | Uses shared AppID and exposes it via IMouldKingDeviceManager. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MouldKing.cs | Registers MouldKing device manager as IMouldKingDeviceManager. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MKBaseNibble.cs | Patches appId bytes into connect/base telegrams via IMouldKingDeviceManager. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MKBaseByte.cs | Patches appId bytes into connect/base telegrams via IMouldKingDeviceManager. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MK3_8.cs | Updates ctor signature to pass IMouldKingDeviceManager to base. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MK4.cs | Updates ctor signature to pass IMouldKingDeviceManager to base. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MK5.cs | Updates ctor signature to pass IMouldKingDeviceManager to base. |
| BrickController2/BrickController2/DeviceManagement/MouldKing/MK6.cs | Updates ctor signature to pass IMouldKingDeviceManager to base. |
| BrickController2/BrickController2.Tests/DeviceManagement/CaDA/CaDADeviceManagerTests.cs | Updates tests to use shared AppID preferences key + service. |
| BrickController2/BrickController2.Tests/DeviceManagement/MouldKing/MouldKingDeviceManagerTests.cs | Updates tests for new DI requirements and adds AppID assertion. |
| BrickController2/BrickController2.Tests/DeviceManagement/JieStar/JieStarDeviceManagerTests.cs | New test verifying JIESTAR uses the shared AppID. |
| BrickController2/BrickController2.Tests/DeviceManagement/DI/VendorBuilderTests.cs | Extends DI tests to register new required manager interfaces for device construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
vicocz
reviewed
Jun 15, 2026
Comment on lines
+35
to
+45
| lock (_lock) | ||
| { | ||
| if (length > _appIdentifier.Length) | ||
| { | ||
| // if the requested length is bigger than the current AppIdentifier, create a new one with the requested length, | ||
| // to keep the AppIdentifier as stable as possible | ||
| _appIdentifier = GetAppIdentifier(_preferencesService, length); | ||
| } | ||
|
|
||
| return _appIdentifier[..length]; | ||
| } |
vicocz
reviewed
Jun 23, 2026
|
|
||
| public ReadOnlyMemory<byte> GetAppId(int length) | ||
| { | ||
| if (length <= 0) |
Owner
There was a problem hiding this comment.
Nit pick: I would simplify it" E.g.
ArgumentOutOfRangeException.ThrowIfNegativeOrZero(length);
| _appIdentifier = GetAppIdentifier(_preferencesService, MIN_APPID_LENGTH); | ||
| } | ||
|
|
||
| public ReadOnlyMemory<byte> GetAppId(int length) |
Owner
There was a problem hiding this comment.
How about using this
IAppIdentifierServiceto simply return 4 / 8 bytes as unique AppId and let all the consumers to use subset as they used it now.WDYT?
Thinging whether we need to auto increasing ID, could we have e.g. 8 bytes by default and throw an execption if a caller wants more?
At the moment we have 2/3 bytes caller.
Your opinion?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As I discovered while reverse‑engineering the JIESTAR devices, the bytes at positions [1, 2] in their datagrams represent an ApplicationIdentifier.
Since JIESTAR and MouldKing devices share the same datagram structure, MouldKing also uses these two bytes as an ApplicationIdentifier.
With this PR, I introduced a global AppIdService, which generates a random ApplicationID on first use and persists it via the PreferencesService.
CaDA, MouldKing, and JIESTAR now all rely on this shared AppIdService.