-
Notifications
You must be signed in to change notification settings - Fork 1
HAL Improvements #212
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
HAL Improvements #212
Conversation
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 implements HAL (Hardware Abstraction Layer) improvements including spelling corrections, bug fixes, and architectural refactoring to support multiple hardware target versions.
- Fixed multiple spelling errors across documentation and comments (e.g., "succesfully" → "successfully", "comunication" → "communication")
- Fixed bugs in USBHostDriver unimplemented methods to return -1 instead of 0 per Arduino Stream API conventions
- Refactored GPIO pin definitions to be target-specific, moving ButtonDrv, Button, and GPIO classes to HALTargetCommon while keeping Pin definitions in target-specific directories
- Added support for new buttons (A, B, C) and LEDs (A, B, C) with pins set to NC (not connected) for V1 hardware
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Service/src/SettingsHandler.h | Fixed spelling: "succesfully" → "successfully" in documentation |
| lib/HALTest/src/MqttClient.h | Fixed spelling: "succesfully" → "successfully" in documentation |
| lib/HALTargetV1/src/USBHostDriver.h | Fixed spelling: "succesfully" → "successfully" in documentation |
| lib/HALTargetV1/src/USBHostDriver.cpp | Fixed bug: return -1 instead of 0 for unimplemented read/peek methods |
| lib/HALTargetV1/src/Pin.h | New file defining target-specific pin mappings for V1 hardware |
| lib/HALTargetCommon/src/Robot.h | Fixed spelling: "comunication" → "communication" |
| lib/HALTargetCommon/src/Network.h | Fixed spelling: "Poit" → "Point" |
| lib/HALTargetCommon/src/MqttClient.h | Fixed spelling: "succesfully" → "successfully", "greather" → "greater" |
| lib/HALTargetCommon/src/MqttClient.cpp | Fixed spelling: "succesfully" → "successfully" and trailing whitespace |
| lib/HALTargetCommon/src/GPIO.h | Refactored to use Pin namespace from external file, added new button/LED pins |
| lib/HALTargetCommon/src/GPIO.cpp | Fixed spelling, added new button/LED pin definitions, removed trailing blank line |
| lib/HALTargetCommon/src/ButtonDrv.h | Added buttons A/B/C, fixed spelling errors, corrected typo "STACKE" → "STACK", improved formatting |
| lib/HALTargetCommon/src/ButtonDrv.cpp | Added buttons A/B/C support, fixed missing buttonIdx reset bug, improved formatting |
| lib/HALTargetCommon/src/Button.h | New file implementing Button class in HALTargetCommon |
| lib/HALTargetCommon/src/Button.cpp | New file implementing Button class methods |
| lib/HALSim/src/Robot.h | Fixed spelling: "comunication" → "communication" |
| lib/HALSim/src/MqttClient.h | Fixed spelling: "succesfully" → "successfully" |
| lib/HALInterfaces/src/IRobot.h | Fixed spelling: "comunication" → "communication" |
| lib/HALInterfaces/src/IMqttClient.h | Fixed spelling: "succesfully" → "successfully" |
| lib/APPTurtle/src/Subscriber.h | Fixed spelling: "succesfully" → "successfully" |
| lib/APPTurtle/src/MicroRosClient.h | Fixed spelling: "succesfully" → "successfully" and trailing whitespace |
| README.md | Fixed spelling: "succesfully" → "successfully" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@BlueAndi currently we are getting a instance of each LED though the board class in order to enable/disable it. The same applies to the buttons. It is possible to change this, and instead provide setters for the LEDs and getters for the buttons directly from the board. This way it would not be necessary to create a specific class Pseudo-Code suggestion: void Board::enableLed(LedId idx, bool enableIt)
{
value = LOW;
if (true == enableIt)
{
value = HIGH;
}
switch(idx)
{
LED_ID_A:
GpioPins::ledAPin.write(value);
...
}
};bool Board::isButtonPressed(ButtonId idx)
{
return (BUTTON_STATE_PRESSED == ButtonDrv::getInstance().getState(idx));
}; |
But its by design the idea to collect the drivers in the Board class without implementing there the logic. Its single source without switch/case logic. |
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
Copilot reviewed 57 out of 58 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
lib/HALTargetCommon/src/GPIO.cpp:28
- Fixed spelling: "Abtraction" corrected to "Abstraction".
lib/HALTargetCommon/src/ButtonDrv.h:149 - Fixed spelling: "notifyed" corrected to "notified".
lib/HALTargetCommon/src/ButtonDrv.h:165 - Fixed spelling: "occurre" corrected to "occur".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BlueAndi
left a comment
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.
All files in the repository missing the doxygen @file attribute which is required for using grouping IMHO. Can you add it to these files already?
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
Copilot reviewed 57 out of 58 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BlueAndi
left a comment
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.
Not all issues solved.
|
I propose changing adding the |
Fixed several typos and bugfixes suggested by Copilot.
Move ButtonDrv, Button and GPIO to HALTargetCommon. Pin definitions are specific to each Target Version.
Added new Pins for compatibility with V2
ToDo: