Fetch dependencies, format, and compile #7
Fetch dependencies, format, and compile #7davidkennedydev wants to merge 6 commits intoOpen-Bionics:masterfrom
Conversation
WalkthroughThis update introduces a comprehensive code style and repository structure overhaul. It adds configuration files for formatting ( Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Repository
participant Submodules
participant BuildSystem
Developer->>Repository: Clone repository
Repository->>Submodules: Initialise and update submodules
Developer->>BuildSystem: Run `make`
BuildSystem->>Repository: Use Makefile and source files
BuildSystem->>Submodules: Include dependency code
BuildSystem-->>Developer: Produce build output
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
.gitignore (2)
1-4: Consider broadening ignored editor/OS artefacts.The current list covers build artefacts, Vim swap files and Clang compilation output, but overlooks other ubiquitous temporary files (e.g. macOS
.DS_Store, JetBrains.idea/, VS Code.vscode/, Emacs*~/.#*, etc.). Adding them keeps accidental noise out of future commits.*.swp +/.*.sw? +.DS_Store +.idea/ +.vscode/ +*~ +# Emacs lockfiles +.#* /build compile_commands.json .cache/
4-5: Ensure file ends with a newline.Posix tools expect text files to terminate with a newline. Please confirm the final blank line is present in the committed blob.
deps/README.md (1)
1-4: Enrich dependency instructions for newcomers.Stating purpose is helpful, but adding a short snippet on how to fetch/update submodules (
git submodule update --init --recursive) and the policy for bumping commit hashes will reduce friction for first-time contributors.This directory contains the dependencies needed to build the project + +## Initialising / updating + +```bash +git submodule update --init --recursive +``` + +Run the above after cloning or whenever the `.gitmodules` file changes. + +## Updating a dependency + +Dependencies are locked to specific commit hashes. +To bump a dependency: + +1. `cd deps/<dependency>` +2. `git fetch && git checkout <new_hash>` +3. `cd ../.. && git add deps/<dependency>` +4. Create a dedicated PR explaining the change.deps/Almond_PCB (1)
1-1: Prefer pinning to a stable, versioned tag rather than a raw commit IDReferencing a bare commit hash can break reproducibility if the upstream history is rewritten or when you later update other submodules. If Almond_PCB offers release tags, consider switching the pointer to the latest stable tag to make future bisects and CI builds deterministic. Otherwise, please document in the README why this specific SHA was chosen.
README.md (3)
33-38: Fix grammar, typo, and add code-block language identifierSuggested improvements:
- Missing comma after the introductory clause.
- Typo: “dependecies” → “dependencies”.
- Add a language identifier (
bash) to the fenced command block to silence MD040.-For **the first time** you clone the project fetch the dependecies using the following `git` command: - -``` +For **the first time** you clone the project, fetch the dependencies using the following `git` command: + +```bash git submodule update --init--- `39-43`: **Specify language for the second fenced block** Markdown-lint (MD040) flags code fences without a language. Adding `bash` also improves readability. ```diff -``` -make -``` +```bash +make +```
35-37: Consider--recursivefor nested submodulesIf any of the declared submodules themselves contain submodules (Arduino cores often do), developers will need
--recursiveon first clone:git submodule update --init --recursiveMentioning this now saves future confusion.
.gitmodules (1)
1-9: Use HTTPS URLs to ease cloning in CI and read-only environmentsSSH URLs require an uploaded key; many CI systems and casual contributors lack this, leading to clone failures. Switching to
https://github.com/...keeps write permissions unchanged (push via SSH can still be configured inside each submodule).-url = git@github.com:Open-Bionics/Almond_PCB.git +url = https://github.com/Open-Bionics/Almond_PCB.git(Apply similarly to the other two entries.)
src/timers/timer_and_delay.h (1)
120-121: Redundant in-class initialiser
_startedis already reset in the constructor; duplicating the initialisation here adds maintenance overhead without benefit.- bool _started = false; /**< flag to indicate if the timer has started */ + bool _started; /**< flag to indicate if the timer has started */(Not critical, but simplifies the code.)
src/timers/samd_FingerTimer.h (1)
44-49: Macro arithmetic comment: clarify units
ms(val)multiplies byTIMER_FREQ, butTIMER_FREQis expressed in kHz. A short note clarifying “ticks per millisecond” prevents mis-interpretation.-#define ms(val) ((val) * (TIMER_FREQ)) // number of timer ticks per ms +#define ms(val) ((val) * (TIMER_FREQ)) // TIMER_FREQ is in kHz → ticks per mssrc/timers/avr_FingerTimer.h (1)
50-66: Nested ternary macro still hard to readAlthough outside this PR’s scope, consider replacing the deeply-nested
PWM_pin_to_timer()macro with a table lookup orconstexprhelper for readability and maintainability.src/FingerLib.h (2)
60-64: Prefer strongly-typedenum classover macro constants for statesUsing pre-processor macros (
CLOSE,OPEN,RIGHT,LEFT) leaks names globally, provides no type-safety, and cannot be inspected by a debugger. Consider replacing them with a scopedenum class(or at least a plainenum) so that the compiler can catch accidental mix-ups and implicit conversions.
128-133: Replace thewindowmacro with aconstexprinline functionThe current macro:
#define window(amt, low, high) ((amt) < (low) ? (0) : ((amt) > (high) ? (0) : (amt)))evaluates its arguments multiple times and inherits all macro pitfalls (missing type safety, no namespace, hard to debug). A zero-cost alternative in modern C++ is:
-#define window(amt, low, high) ((amt) < (low) ? (0) : ((amt) > (high) ? (0) : (amt))) +template <typename T> +constexpr T window(T amt, T low, T high) { + return (amt < low || amt > high) ? static_cast<T>(0) : amt; +}This avoids side-effects, respects overload resolution, and keeps the symbol inside the
FingerLibnamespace once placed in a header.src/current_sense/samd_CurrentSense.cpp (2)
26-27: Usenullptrinstead ofNULL
nullptris the idiomatic C++ way to represent a null pointer, avoids the implicit integer conversion ofNULL, and is already available in all compiler versions that supportautoformatting here.
280-289: Declare the IRQ lookup tableconstexpr/staticto avoid repeated stack copies
IRQn_Type timeIRQnList[NUM_TIMERS]is reconstructed every call toenableCurrentSnsTimer. Marking itstatic constexpr(or moving it to file scope) removes the minor runtime overhead and clarifies its immutability:-IRQn_Type timeIRQnList[NUM_TIMERS] = { ... }; +static constexpr IRQn_Type timeIRQnList[NUM_TIMERS] = { ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.clang-format(1 hunks).gitignore(1 hunks).gitmodules(1 hunks)Makefile(1 hunks)README.md(1 hunks)deps/Almond_PCB(1 hunks)deps/ArduinoCore-API(1 hunks)deps/ArduinoCore-avr(1 hunks)deps/README.md(1 hunks)src/FingerLib.cpp(5 hunks)src/FingerLib.h(1 hunks)src/buffer/CircleBuff.h(2 hunks)src/current_sense/samd_CurrentSense.cpp(2 hunks)src/current_sense/samd_CurrentSense.h(1 hunks)src/pid/pid_controller.cpp(1 hunks)src/pid/pid_controller.h(1 hunks)src/timers/avr_FingerTimer.cpp(1 hunks)src/timers/avr_FingerTimer.h(2 hunks)src/timers/samd_FingerTimer.cpp(1 hunks)src/timers/samd_FingerTimer.h(2 hunks)src/timers/timer_and_delay.cpp(1 hunks)src/timers/timer_and_delay.h(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/timers/timer_and_delay.h (1)
src/timers/timer_and_delay.cpp (4)
NB_DELAY_CLASS(17-20)NB_DELAY_CLASS(22-22)NB_TIMER_CLASS(92-98)NB_TIMER_CLASS(100-100)
src/timers/timer_and_delay.cpp (1)
src/timers/timer_and_delay.h (7)
NB_DELAY_CLASS(40-40)NB_DELAY_CLASS(41-41)delVal(43-43)delVal(49-49)NB_TIMER_CLASS(100-100)NB_TIMER_CLASS(101-101)interval(111-111)
src/timers/avr_FingerTimer.cpp (1)
src/timers/avr_FingerTimer.h (3)
f(71-71)f(72-72)_changePWMFreq(69-69)
🪛 LanguageTool
README.md
[uncategorized] ~33-~33: Possible missing preposition found.
Context: ...the first time* you clone the project fetch the dependecies using the following `gi...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...e --init ``` To build and rebuild the project use the following GNU Make command: ``...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
README.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 checkmake (0.2.2)
Makefile
[warning] 17-17: Missing required phony target "all"
(minphony)
[warning] 17-17: Missing required phony target "test"
(minphony)
[warning] 19-19: Target "all" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (19)
deps/ArduinoCore-API (1)
1-1: Consider referencing a tagged release instead of a raw commit hashLocking the submodule to a specific commit guarantees reproducibility, but using an official tag (e.g. a released version) makes future maintenance and security tracking easier. If a suitable tag exists, updating
.gitmodulesto reference it would improve clarity for downstream contributors.Please double-check whether ArduinoCore-API provides a stable release tag that matches
4a02bfc. If so, switching to that tag would be preferable; otherwise, documenting why this exact commit is required would help future maintainers.deps/ArduinoCore-avr (1)
1-1: Submodule commit pointer added – please verify remote availability and licensing.The gitlink correctly pins the dependency to a specific commit, which is good for reproducibility.
Before merging, double-check that the URL configured in.gitmodulesremains reachable and that thec8c514c…commit is still present upstream. Also ensure the Arduino core’s licence is compatible with the main project’s licence and acknowledged in your NOTICE/LICENCE files.If everything checks out, no further action is needed.
src/current_sense/samd_CurrentSense.h (1)
18-30: Header refactor is purely stylistic – no action requiredNothing but whitespace and comment-wrapping changes. Behaviour of the helper
syncTC_8()remains identical.src/timers/avr_FingerTimer.h (1)
20-38: Macro block is unchanged semanticallyOnly spacing and comment alignment were touched – timer-frequency constants are intact.
src/timers/avr_FingerTimer.cpp (1)
30-46: Timer setup formatting only – looks goodNo functional differences detected; ISR configuration is intact.
src/timers/samd_FingerTimer.cpp (4)
7-11: LGTM! License header formatting improved.The license header has been properly reformatted with appropriate line breaks, improving readability whilst maintaining all original content.
25-29: LGTM! Function declarations streamlined.Converting these simple function assignments to single-line declarations improves code compactness without sacrificing readability.
32-73: LGTM! Timer setup function formatting enhanced.The consistent indentation and spacing throughout the timer configuration code significantly improves readability whilst preserving all register operations and synchronization logic.
76-115: LGTM! Interrupt handler formatting improved.The interrupt service routine has been reformatted with proper indentation and spacing, making the control flow and timer logic much easier to follow.
src/buffer/CircleBuff.h (3)
2-15: LGTM! License header formatting standardised.The header comment block has been properly reformatted with consistent line breaks and spacing, improving overall presentation.
24-41: LGTM! Macro definitions and comments formatted consistently.The macro definitions and accompanying comments have been formatted with proper spacing and alignment, making the buffer size options clearer to read.
57-257: LGTM! Template class formatting enhanced.The CIRCLE_BUFFER template class has been reformatted with consistent indentation, proper method spacing, and improved comment alignment throughout. All functionality remains unchanged whilst significantly improving readability.
src/timers/timer_and_delay.cpp (3)
2-11: LGTM! License header formatting improved.The license header has been reformatted with consistent line breaks and spacing.
17-88: LGTM! NB_DELAY_CLASS formatting enhanced.All methods in the NB_DELAY_CLASS hierarchy have been reformatted with consistent indentation, spacing, and brace placement, significantly improving code readability whilst preserving all timer logic.
92-177: LGTM! NB_TIMER_CLASS formatting enhanced.The NB_TIMER_CLASS methods have been consistently formatted with proper indentation and spacing, making the timer management logic much easier to follow.
src/pid/pid_controller.h (3)
2-11: LGTM! License header formatting standardised.The header comment block has been reformatted with consistent spacing and line breaks.
17-41: LGTM! Include directives and macro definitions formatted consistently.The include statements and macro definitions have been formatted with proper spacing, and the commented macro options are now properly aligned for better readability.
43-77: LGTM! Class declaration formatting enhanced.The PID_CONTROLLER class declaration has been reformatted with consistent indentation and proper spacing between method declarations and member variables, improving overall code organisation.
.clang-format (1)
1-288: LGTM! Comprehensive and well-configured formatting rules.This .clang-format configuration establishes excellent formatting standards for the project:
- 2-space indentation with 80-character line limit promotes readability
- Consistent brace wrapping and spacing rules enhance code uniformity
- Right-aligned pointers follow common C++ conventions
- Disabled consecutive alignments prevent excessive formatting churn
- Comprehensive coverage of C++ language features ensures consistent formatting
The configuration is well-suited for an embedded C++ project and will maintain consistent code style across all contributors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Makefile (2)
17-19: Declare all relevant targets as phony
all(and usuallybuild) are not real files and should be marked.PHONYto prevent make from confusing stale artefacts with targets. This also addresses thecheckmakewarnings.-.PHONY: clean +.PHONY: all build clean
14-15: Quote the*.cpppattern to avoid premature shell glob expansionUnquoted wildcards are expanded by the shell before
findruns, which breaks if no.cppfiles exist in the working directory (the pattern ends up as extra arguments tofind). Quote the pattern sofindreceives it verbatim.-EXECUTABLES= $(shell find src -type f -name *.cpp \ +EXECUTABLES = $(shell find src -type f -name '*.cpp' \
🧹 Nitpick comments (1)
Makefile (1)
3-12: Remove trailing back-slashes on the final line of multiline variable assignmentsLeaving a back-slash on the last line continues the logical line into the following (blank) line, inadvertently inserting a stray space and making future edits error-prone. Trim the back-slash after the final list item in
MCU_FLAGSandARDUINO_INCLUDE.- -D F_CPU=16000000UL \ + -D F_CPU=16000000UL … - -I deps/ArduinoCore-avr/variants/mega/ \ + -I deps/ArduinoCore-avr/variants/mega/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 17-17: Missing required phony target "all"
(minphony)
[warning] 17-17: Missing required phony target "test"
(minphony)
[warning] 19-19: Target "all" should be declared PHONY.
(phonydeclared)
|
@OllyMcBrideOB or @per1234, could you have a look? I'm looking to contribute way more after this first simple PR. |
1 participant
Modernize the project
NO CODE CHANGES BEYOND FORMATTING WERE MADE
Summary by CodeRabbit
New Features
Style
Chores