-
Notifications
You must be signed in to change notification settings - Fork 18
Updater Improvements and Refactoring #113
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
base: main
Are you sure you want to change the base?
Conversation
There is no reason to log/print the stack-trace on `CliInvalidException` (logic errors, e.g. IP-Secure together with USB-Interface)
Instead of creating/configuring/tinkering by our self a fatJar, we use now `com.gradleup.shadow` to create a `shadowJar`. The old `fatJar` gradle task will be now redirected to the new `shadowJar` task for some time.
…IDEA) downloaded on 10.08.2024 from [Github gitignore - JetBrains.gitignore](https://github.com/github/gitignore/blob/main/Global/JetBrains.gitignore) based on [JetBrains IDEs Support IntelliJ Platform - How to manage projects under Version Control Systems](https://intellij-support.jetbrains.com/hc/en-us/articles/206544839-How-to-manage-projects-under-Version-Control-Systems)
This commit breaks intentionally the build to fix the code base folder hierarchy and not to loose git history. Next commits will (hopefully) fix it. Reasons for code base reorganization: - The Updater project itself was in subfolder `source`, which is redundant since we already use `src` for source files - `main` code was in `source/src/main/java/org/selfbus/*`. - `test` code was in `source/src/test/java/org/selfbus/*`.
New files in `.idea` are from a fresh gradle import in IntelliJ IDEA Community Edition 2024.2
`objno` was logged instead of `objConf`
The 7th bit of the Config Octet shall always be 1 for Group Object Table - Realisation Type 1 (KNX Spec. 3.0 3/5/1 4.18.3.1.2.1). The 7th bit of the Config Octet of The Group Object Table - Realisation Type 2 is used as Update Enable flag (KNX Spec. 3.0 3/5/1 4.18.4.1).
Also fixed gradle warning: The archives configuration has been deprecated for artifact declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces substantial improvements to the firmware updater, focusing on reliability, maintainability, and user experience. The changes include:
- Enhanced connection handling with new reconnect options
- Improved KNX interface discovery
- Reorganized logging system with custom filters
- Major code refactoring with relocated source structure
- Migration from JUnit 4 to JUnit 5
- Updated dependencies and build configuration
Key Changes
- Refactored communication object handling logic with improved APCI command processing
- Added comprehensive test coverage for logging, GUI components, and utility functions
- Migrated test framework from JUnit 4 to JUnit 5 with Hamcrest replaced by native assertions
- Enhanced debug output with new helper functions for communication object inspection
Reviewed changes
Copilot reviewed 130 out of 158 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| test/lib-test-cases/src/knx/test_*.{h,cpp} | New test files for memory and EEPROM functionality |
| sblib/src/eib/com_objectsBCU1.cpp | Refactored processGroupTelegram logic with switch-case pattern |
| sblib/src/eib/com_object*.cpp | Added debug output functions and formatting improvements |
| sblib/inc/sblib/eib/types.h | Added COMCONF_UPDATE enum value |
| firmware_updater/updater/test/**/*.java | Migrated to JUnit 5 and added new test coverage |
| firmware_updater/updater/test/resources/hex/*.hex | Updated test resources with corrected paths |
Files not reviewed (10)
- firmware_updater/updater/.idea/.name: Language not supported
- firmware_updater/updater/.idea/Selfbus_Updater.iml: Language not supported
- firmware_updater/updater/.idea/artifacts/SB_updater_main_jar.xml: Language not supported
- firmware_updater/updater/.idea/compiler.xml: Language not supported
- firmware_updater/updater/.idea/misc.xml: Language not supported
- firmware_updater/updater/.idea/modules.xml: Language not supported
- firmware_updater/updater/.idea/modules/SB_updater.main.iml: Language not supported
- firmware_updater/updater/.idea/modules/SB_updater.test.iml: Language not supported
- firmware_updater/updater/.idea/runConfigurations/_template__of_Application.xml: Language not supported
- firmware_updater/updater/source/.idea/artifacts/SB_updater_main_jar.xml: Language not supported
Comments suppressed due to low confidence (1)
firmware_updater/updater/src/org/selfbus/updater/bootloader/BootloaderStatistic.java:36
- This method overrides Object.toString; it is advisable to add an Override annotation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| private class SBTransportListener implements TransportListener |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SBTransportListener could be made static, since the enclosing instance is used only in its constructor.
|
|
||
| if (cmdLine.hasOption(OPT_LONG_DUMPFLASH)) { | ||
| String[] optArgs = cmdLine.getOptionValues(OPT_LONG_DUMPFLASH); | ||
| setDumpFlashStartAddress(Long.decode(optArgs[0])); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential uncaught 'java.lang.NumberFormatException'.
| if (cmdLine.hasOption(OPT_LONG_DUMPFLASH)) { | ||
| String[] optArgs = cmdLine.getOptionValues(OPT_LONG_DUMPFLASH); | ||
| setDumpFlashStartAddress(Long.decode(optArgs[0])); | ||
| setDumpFlashEndAddress(Long.decode(optArgs[1])); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential uncaught 'java.lang.NumberFormatException'.
| int cursorMovementCode = cursorMovement.charAt(cursorMovement.length() - 1); | ||
| int number; | ||
| if (cursorMovement.length() > 1) { | ||
| number = Integer.parseInt(cursorMovement.substring(0, cursorMovement.length() - 1)); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential uncaught 'java.lang.NumberFormatException'.
| code = code.substring(0, code.length() - 1); // strip of 'm' | ||
| } | ||
|
|
||
| switch (Integer.parseInt(code)) { |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential uncaught 'java.lang.NumberFormatException'.
| } | ||
| } | ||
|
|
||
| public void run() { |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method overrides Thread.run; it is advisable to add an Override annotation.
| * | ||
| * @see java.lang.Runnable#run() | ||
| */ | ||
| public void run() { |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method overrides Runnable.run; it is advisable to add an Override annotation.
| this.linkLogger = (Logger) LoggerFactory.getLogger(SBLinkListener.class.getName() + " " + link.getName()); | ||
| } | ||
|
|
||
| public void indication(final FrameEvent e) |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method overrides LinkListener.indication; it is advisable to add an Override annotation.
| return new BootloaderIdentity(vMajor, vMinor, versionSBLibMajor, versionSBLibMinor, features, applicationFirstAddress); | ||
| } | ||
|
|
||
| public String toString() { |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method overrides Record.toString; it is advisable to add an Override annotation.
| return " Done Speed Avg Min Max Time"; | ||
| } | ||
|
|
||
| public String toString() { |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method overrides Object.toString; it is advisable to add an Override annotation.
This PR contains improvements to the firmware updater focusing on reliability, maintainability and user experience.
Key Features & Enhancements
Connection & Reliability:
--ip-tunnel-reconnectoption for IP interfaces with non-standard L2 sequence number handling (e.g., Loxone Miniserver Gen1)--reconnectdelay (milliseconds)Discovery & Interface Detection:
--discoverargument to list available KNX interfaces (IP, USB and serial-port) and their propertiesLogging & Output:
--versionCode Quality & Architecture:
source/srctosrc/)shadowJarplugin for fat JAR creationDeviceManagementFactoryfor reflection-based L2 handling of reconnecting.logging,.progress,.devicemgntCliInvalidExceptionfor validation errorsGUI Improvements:
settings.jsonLoggingPane: JTextPaneUncaughtExceptionHandlerBug Fixes:
buffernot repeated if error occurred in last while run 4e7152eBreaking Changes
source/src, nowsrc/)Testing
Added test coverage for logging components, evaluators and core functionality.
Dependency Updates: