Open
Conversation
Reviewer's GuideAdds UDP-based mirroring of the UI text output alongside the existing LCD display, wiring it into the network initialization path and introducing minimal socket-based send logic. Sequence diagram for DisplayWrite mirroring to LCD and UDPsequenceDiagram
actor User
participant CMiniDexed
participant CUserInterface
participant LCDDevice as CLCD
participant UDPSocket as CSocket
User ->> CMiniDexed: triggers UI update
CMiniDexed ->> CUserInterface: DisplayWrite(pMenu, pParam, pValue)
CUserInterface->>CUserInterface: Build Msg (LCD escape codes + text)
CUserInterface->>CUserInterface: Build Msg2 (Msg + newline, second line, clear-to-end)
CUserInterface ->> CLCD: LCDWrite(Msg)
CUserInterface ->> UDPSocket: UDPWrite(Msg2)
alt UDP socket available
UDPSocket ->> UDPSocket: SendTo(Msg2, length, 0, m_UDPDestAddress, m_UDPDestPort)
else UDP socket not available
CUserInterface ->> CUserInterface: Skip UDP send
end
Sequence diagram for network initialization and UDP UI setupsequenceDiagram
participant CMiniDexed
participant CUserInterface
participant NetSubSystem as CNetSubSystem
participant UDPSocket as CSocket
participant CIPAddress
CMiniDexed ->> CMiniDexed: UpdateNetwork()
CMiniDexed ->> CUserInterface: InitUDP()
CUserInterface ->> CIPAddress: Set(0x2c00000a)
CUserInterface ->> CUserInterface: m_UDPDestPort = 1306
CUserInterface ->> CIPAddress: IsNull()
alt destination address is valid
CUserInterface ->> NetSubSystem: Get()
NetSubSystem -->> CUserInterface: CNetSubSystem*
CUserInterface ->> UDPSocket: new CSocket(pNet, IPPROTO_UDP)
CUserInterface ->> UDPSocket: Connect(m_UDPDestAddress, m_UDPDestPort)
CUserInterface ->> UDPSocket: SetOptionBroadcast(TRUE)
else destination address is null
CUserInterface ->> CUserInterface: UDP display disabled
end
CUserInterface -->> CMiniDexed: InitUDP() returns true
Class diagram for updated CUserInterface UDP display supportclassDiagram
class CMiniDexed {
+void UpdateNetwork()
-bool m_bNetworkReady
-CUserInterface m_UI
}
class CUserInterface {
+bool Initialize()
+bool InitUDP()
+void Process()
+void ParameterChanged()
+void DisplayWrite(const char* pMenu, const char* pParam, const char* pValue)
-void LCDWrite(const char* pString)
-void UDPWrite(const char* pString)
-CMiniDexed* m_pMiniDexed
-CConfig* m_pConfig
-CCharacterDevice* m_pLCD
-CWriteBufferDevice* m_pLCDBuffered
-CSocket* m_pUDPSendSocket
-CIPAddress m_UDPDestAddress
-unsigned m_UDPDestPort
}
class CNetSubSystem {
+static CNetSubSystem* Get()
}
class CSocket {
+CSocket(CNetSubSystem* pNet, unsigned protocol)
+bool Connect(CIPAddress address, unsigned port)
+bool SetOptionBroadcast(bool enable)
+int SendTo(const void* buffer, unsigned length, unsigned flags, CIPAddress address, unsigned port)
}
class CIPAddress {
+void Set(unsigned address)
+void Format(CString* pString)
+bool IsNull()
}
CMiniDexed --> CUserInterface : owns
CUserInterface ..> CNetSubSystem : uses
CUserInterface ..> CSocket : uses
CUserInterface ..> CIPAddress : uses
CSocket ..> CNetSubSystem : constructed_with
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider avoiding the hard-coded UDP destination (0x2c00000a:1306) and aligning behavior with the existing configuration model and the 0.0.0.0 ‘disabled’ convention, so runtime configuration and default broadcast work as described in the comments.
- m_pUDPSendSocket is allocated in InitUDP but never freed; it would be good to either own and clean it up in CUserInterface’s destructor or switch to a RAII-style wrapper to avoid leaking the socket.
- DisplayWrite currently builds two largely duplicated strings (Msg and Msg2) with slightly different control sequences; consider refactoring to share the common formatting logic or clearly separate the terminal vs UDP protocols to avoid them drifting out of sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding the hard-coded UDP destination (0x2c00000a:1306) and aligning behavior with the existing configuration model and the 0.0.0.0 ‘disabled’ convention, so runtime configuration and default broadcast work as described in the comments.
- m_pUDPSendSocket is allocated in InitUDP but never freed; it would be good to either own and clean it up in CUserInterface’s destructor or switch to a RAII-style wrapper to avoid leaking the socket.
- DisplayWrite currently builds two largely duplicated strings (Msg and Msg2) with slightly different control sequences; consider refactoring to share the common formatting logic or clearly separate the terminal vs UDP protocols to avoid them drifting out of sync.
## Individual Comments
### Comment 1
<location> `src/userinterface.cpp:59` </location>
<code_context>
+{
+ // UDP MIDI send socket setup (default: broadcast 255.255.255.255:1999)
+ //m_UDPDestAddress.Set(0xFFFFFFFF); // Broadcast by default
+ m_UDPDestAddress.Set(0x2c00000a); //
+ m_UDPDestPort = 1306;
+#if 0
</code_context>
<issue_to_address>
**suggestion:** The hard-coded IP literal 0x2c00000a is a magic number and makes the target address harder to understand or change.
This raw literal obscures which IP is used and hard-codes it in the logic. Please either source the address from configuration (as the commented-out code implies) or introduce a named constant/helper (e.g., `constexpr uint32_t kDefaultUdpDest = ...;`) so the intent is clear and the value is easy to change later.
Suggested implementation:
```cpp
LOGMODULE ("ui");
delete m_pLCD;
}
namespace {
constexpr uint32_t kDefaultUdpDestAddress = 0x2c00000a; // 10.0.0.44
constexpr uint16_t kDefaultUdpDestPort = 1306;
}
bool CUserInterface::InitUDP (void)
```
```cpp
// UDP MIDI send socket setup (default: broadcast 255.255.255.255:1999)
//m_UDPDestAddress.Set(0xFFFFFFFF); // Broadcast by default
m_UDPDestAddress.Set(kDefaultUdpDestAddress);
m_UDPDestPort = kDefaultUdpDestPort;
```
</issue_to_address>
### Comment 2
<location> `src/userinterface.cpp:327` </location>
<code_context>
}
}
+void CUserInterface::UDPWrite (const char *pString)
+{
+ if (m_pUDPSendSocket) {
</code_context>
<issue_to_address>
**nitpick (bug_risk):** UDPWrite re-computes strlen and uses `%u` with a `size_t` value, which can be tightened up.
Two small tweaks:
- Cache `strlen(pString)` in a local `size_t len` and reuse it for both `SendTo` and logging.
- Since `strlen` returns `size_t`, use `%zu` in the log (or cast to `unsigned`) to avoid a type/format mismatch on some platforms.
</issue_to_address>
### Comment 3
<location> `src/userinterface.cpp:52` </location>
<code_context>
CUserInterface::~CUserInterface (void)
</code_context>
<issue_to_address>
**issue (bug_risk):** The UDP socket allocated in InitUDP is not released in the destructor, leading to a leak.
Please also delete `m_pUDPSendSocket` in the destructor (with a null check if needed) so the UDP socket is properly released.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| void CUserInterface::UDPWrite (const char *pString) |
There was a problem hiding this comment.
nitpick (bug_risk): UDPWrite re-computes strlen and uses %u with a size_t value, which can be tightened up.
Two small tweaks:
- Cache
strlen(pString)in a localsize_t lenand reuse it for bothSendToand logging. - Since
strlenreturnssize_t, use%zuin the log (or cast tounsigned) to avoid a type/format mismatch on some platforms.
| @@ -50,6 +52,37 @@ CUserInterface::~CUserInterface (void) | |||
| delete m_pLCD; | |||
There was a problem hiding this comment.
issue (bug_risk): The UDP socket allocated in InitUDP is not released in the destructor, leading to a leak.
Please also delete m_pUDPSendSocket in the destructor (with a null check if needed) so the UDP socket is properly released.
|
Build for testing: |
The autoreboot feature can be abused as you dont need a valid login to trigger the reboot. This fix will only auto reboot after you quit a logged in session.
explicitly move the cursor to row 2 when drawing menu so I can remove Msg2 and write the same strings to local displays and the udp display UDP remote display can now be configured: UDPDisplayEnabled=0 UDPDisplayIPAddress=10.0.0.44
|
Build for testing: |
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
1 participant
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.
For your review only at this stage.
VERY! incomplete, but it does work.
If you're interested in getting this in I will:
Summary by Sourcery
Add support for mirroring the LCD user interface over UDP when networking is available.
New Features:
Enhancements: