Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRefined select-mode exit condition to permit command Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Device as tComm.c
participant Memory
participant UART
Client->>Device: Send command (type = 0x20)
activate Device
Note over Device: select_mode allows 0x20 (refined exit condition)
Device->>Client: Request 6-byte params
Client->>Device: Send start_addr (3B), length (3B)
rect rgba(235,245,255,0.5)
Note over Device,Memory: Loop: read up to 0x8000 bytes and transmit
loop while remaining > 0
Device->>Memory: Read chunk at current addr
Memory-->>Device: Chunk data
Device->>UART: Transmit chunk
UART-->>Client: Chunk data
end
end
Device->>UART: Send prompt
UART-->>Client: Prompt
deactivate Device
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
|
Artifacts: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Discovery/Src/tComm.c (1)
662-697: Filter change for 0x20 looks correct; keep comment in sync with future service commandsUpdating the exit-condition to
(type < 0x60 && type != 0x20) || type == 0xFFis the right move so the new 0x20 service command is not treated as “text” and mistakenly aborting the session. Including 0x20 in the “return-of-command” switch keeps the handshake consistent with other service commands.Just make sure that any future
< 0x60service commands (e.g. 0x22/0x23/0x30/0x40/0x42/0x50) also update this filter; the new comment is a good reminder, so I’d rely on that going forward.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Discovery/Src/tComm.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Discovery/Src/tComm.c (1)
Common/Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_uart.c (2)
HAL_UART_Receive(696-774)HAL_UART_Transmit(614-685)
mikeller
force-pushed
the
add_memory_reading
branch
from
2056769 to
cb60c84
Compare
|
Artifacts: |
mikeller
force-pushed
the
add_memory_reading
branch
from
cb60c84 to
e548a7f
Compare
|
Artifacts: |
mikeller
force-pushed
the
development
branch
from
1811b4a to
d0f337c
Compare
mikeller
force-pushed
the
add_memory_reading
branch
from
e548a7f to
46c34eb
Compare
|
Artifacts: |
There was a problem hiding this comment.
Pull request overview
This PR implements Command 0x20 to enable reading arbitrary memory blocks over UART communication. The implementation adds support for reading data from specified memory addresses with chunked transfers.
Key Changes:
- Modified command exit logic to allow 0x20 command to be processed rather than prematurely exiting
- Added case 0x20 handler that receives 6 bytes (3-byte address + 3-byte length), validates input, and transmits memory contents in 32KB chunks
- Updated comments to document the new command and remind developers to update the filter when adding new commands
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint32_t address = ((uint32_t)aRxBuffer[0] << 24) | ||
| | ((uint32_t)aRxBuffer[1] << 16) | ||
| | ((uint32_t)aRxBuffer[2] << 8); |
There was a problem hiding this comment.
The address calculation appears incorrect. Given that 6 bytes are received, and based on the pattern of the length calculation using bytes 3-5, the address should use bytes 0-2. For a 3-byte address (24 bits), the typical big-endian format would be:
- aRxBuffer[0] << 16 (bits 23-16, MSB)
- aRxBuffer[1] << 8 (bits 15-8)
- aRxBuffer[2] (bits 7-0, LSB)
However, the current code has:
- aRxBuffer[0] << 24 (bits 31-24)
- aRxBuffer[1] << 16 (bits 23-16)
- aRxBuffer[2] << 8 (bits 15-8)
- Missing the LSB (bits 7-0)
This means the address is always missing its least significant byte and is shifted left by 8 bits. This should be corrected to properly parse the 3-byte address.
| uint32_t address = ((uint32_t)aRxBuffer[0] << 24) | |
| | ((uint32_t)aRxBuffer[1] << 16) | |
| | ((uint32_t)aRxBuffer[2] << 8); | |
| uint32_t address = ((uint32_t)aRxBuffer[0] << 16) | |
| | ((uint32_t)aRxBuffer[1] << 8) | |
| | (uint32_t)aRxBuffer[2]; |
| address += chunkSize; | ||
| } | ||
|
|
||
| aTxBuffer[count++] = prompt4D4C(receiveStartByteUart); |
There was a problem hiding this comment.
The count variable is initialized to 0 at the beginning of the function but is never set before being used here with the post-increment operator. This will set aTxBuffer[0] to the prompt value. However, looking at the pattern used in other cases (e.g., lines 758, 982, 985), the prompt should be added to the buffer, and count should track how many bytes are in the buffer. This appears correct, but the issue is that there's no transmission of aTxBuffer before the break, unlike other similar cases. The transmission at line 1023 only happens if count > 0. Consider whether the prompt should be transmitted immediately here or if this follows the expected pattern.
| if ((endAddress - address) < 0x8000) { | ||
| chunkSize = endAddress - address; | ||
| } | ||
| if (HAL_UART_Transmit(&UartHandle, (uint8_t*)address, chunkSize, 60000) != HAL_OK) |
There was a problem hiding this comment.
Casting an arbitrary integer address directly to a pointer and then transmitting from that memory location poses a serious security risk. This allows reading from any memory address in the system, including potentially sensitive data, security keys, or protected memory regions. There should be validation to ensure that the address falls within allowed memory ranges (e.g., specific flash regions, RAM regions that are safe to read). Consider implementing an address validation function that checks against a whitelist of allowed memory regions before performing the read operation.
| uint32_t address = ((uint32_t)aRxBuffer[0] << 24) | ||
| | ((uint32_t)aRxBuffer[1] << 16) | ||
| | ((uint32_t)aRxBuffer[2] << 8); | ||
| uint32_t length = ((uint32_t)aRxBuffer[3] << 16) | ||
| | ((uint32_t)aRxBuffer[4] << 8) | ||
| | (uint32_t)aRxBuffer[5]; |
There was a problem hiding this comment.
The variable declarations are placed after executable statements (the HAL_UART_Receive call), which violates C89/C90 standards. While this is valid in C99 and later, it may cause compilation issues if the project is configured to use an older C standard. Consider moving the variable declarations to the beginning of the case block or to the beginning of the function to ensure compatibility.
| if ((endAddress - address) < 0x8000) { | ||
| chunkSize = endAddress - address; |
There was a problem hiding this comment.
The chunk size is defined as uint16_t with a maximum value of 0x8000 (32768), but when the remaining data is less than 0x8000, the assignment 'chunkSize = endAddress - address' could potentially overflow if the difference exceeds 65535. While the current logic with the if condition should prevent this, it would be safer and clearer to use an explicit cast or ensure the value fits: 'chunkSize = (uint16_t)(endAddress - address)'.
| if (HAL_UART_Transmit(&UartHandle, (uint8_t*)address, chunkSize, 60000) != HAL_OK) | ||
| return 0; |
There was a problem hiding this comment.
Inconsistent indentation: this line uses a tab character while surrounding code uses spaces. The code should use consistent indentation throughout (either all tabs or all spaces). Looking at the surrounding code, spaces appear to be the standard.
| if (HAL_UART_Transmit(&UartHandle, (uint8_t*)address, chunkSize, 60000) != HAL_OK) | |
| return 0; | |
| if (HAL_UART_Transmit(&UartHandle, (uint8_t*)address, chunkSize, 60000) != HAL_OK) | |
| return 0; |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Discovery/Src/tComm.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (3)
Discovery/Src/tComm.c (3)
662-663: LGTM!The filter logic correctly allows command 0x20 through while maintaining the block for other commands below 0x60.
668-676: LGTM!Helpful comment reminding developers to adjust the filter when adding new service commands, and 0x20 is correctly added to the acknowledgment switch.
720-743: LGTM - previous review concerns addressed.The implementation properly validates zero-length requests, checks for 32-bit overflow, streams memory in 0x8000-byte chunks, and correctly handles UART transmission failures by returning early. The prompt is sent only after successful transfer completion.
| uint32_t address = ((uint32_t)aRxBuffer[0] << 24) | ||
| | ((uint32_t)aRxBuffer[1] << 16) | ||
| | ((uint32_t)aRxBuffer[2] << 8); | ||
| uint32_t length = ((uint32_t)aRxBuffer[3] << 16) | ||
| | ((uint32_t)aRxBuffer[4] << 8) | ||
| | (uint32_t)aRxBuffer[5]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the relevant section of tComm.c
fd "tComm.c" --type fRepository: mikeller/ostc4
Length of output: 79
🏁 Script executed:
# Get the file to check its size and read the relevant section
wc -l Discovery/Src/tComm.cRepository: mikeller/ostc4
Length of output: 84
🏁 Script executed:
# Read lines around 713-718 to see the actual code and context
sed -n '700,730p' Discovery/Src/tComm.cRepository: mikeller/ostc4
Length of output: 992
🏁 Script executed:
# Search for protocol documentation or comments about address/length encoding
rg -A 5 -B 5 "aRxBuffer\[0\].*24" Discovery/Src/tComm.cRepository: mikeller/ostc4
Length of output: 583
🏁 Script executed:
# Look for how the address and length values are used after parsing
sed -n '713,750p' Discovery/Src/tComm.cRepository: mikeller/ostc4
Length of output: 1370
🏁 Script executed:
# Search for protocol documentation or specification comments
rg -B 10 "case 0x20:" Discovery/Src/tComm.c | head -30Repository: mikeller/ostc4
Length of output: 1163
🏁 Script executed:
# Look for any comments mentioning address format or alignment
rg -i "address.*align|align.*address|256.*byte|byte.*align" Discovery/Src/tComm.cRepository: mikeller/ostc4
Length of output: 202
🏁 Script executed:
# Search for protocol specification or documentation files
fd -e md -e txt -e h | xargs rg -l "0x20\|memory.*block\|address.*length" 2>/dev/null | head -10Repository: mikeller/ostc4
Length of output: 40
🏁 Script executed:
# Check git history for this section to see if there are comments in commits
git log --oneline -p Discovery/Src/tComm.c | grep -A 20 "case 0x20:" | head -60Repository: mikeller/ostc4
Length of output: 1725
Address parsing is asymmetrically encoded—likely a bug.
The address and length are parsed from the 6-byte payload using different shift patterns despite using the same number of bytes (3 each):
- Address:
<< 24, << 16, << 8— forces lower 8 bits to 0 (256-byte alignment) - Length:
<< 16, << 8, << 0— uses all 24 bits
Since the address is immediately cast to (uint8_t*) and used as a raw memory pointer, forcing 256-byte alignment appears unintentional. This suggests a copy-paste error where the shift positions weren't adjusted after the length pattern was established.
Correct to match the length pattern for consistent byte-level addressing:
Proposed fix
- uint32_t address = ((uint32_t)aRxBuffer[0] << 24)
- | ((uint32_t)aRxBuffer[1] << 16)
- | ((uint32_t)aRxBuffer[2] << 8);
+ uint32_t address = ((uint32_t)aRxBuffer[0] << 16)
+ | ((uint32_t)aRxBuffer[1] << 8)
+ | (uint32_t)aRxBuffer[2];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint32_t address = ((uint32_t)aRxBuffer[0] << 24) | |
| | ((uint32_t)aRxBuffer[1] << 16) | |
| | ((uint32_t)aRxBuffer[2] << 8); | |
| uint32_t length = ((uint32_t)aRxBuffer[3] << 16) | |
| | ((uint32_t)aRxBuffer[4] << 8) | |
| | (uint32_t)aRxBuffer[5]; | |
| uint32_t address = ((uint32_t)aRxBuffer[0] << 16) | |
| | ((uint32_t)aRxBuffer[1] << 8) | |
| | (uint32_t)aRxBuffer[2]; | |
| uint32_t length = ((uint32_t)aRxBuffer[3] << 16) | |
| | ((uint32_t)aRxBuffer[4] << 8) | |
| | (uint32_t)aRxBuffer[5]; |
🤖 Prompt for AI Agents
In Discovery/Src/tComm.c around lines 713-718, the 3-byte address is incorrectly
shifted as <<24,<<16,<<8 which zeroes the low byte and enforces 256-byte
alignment; change the address parsing to use the same 3-byte pattern as length
(<<16, <<8, <<0) so the full 24-bit address is reconstructed correctly from
aRxBuffer[0..2]; ensure the resulting uint32_t is then safely cast/used as
(uint8_t*) where appropriate and consider validating the parsed address range
before dereferencing.
46c34eb to
e27b0c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @Discovery/Src/tComm.c:
- Line 733: The compilation error is from a typo in the cast type on the
assignment to chunkSize: change the mistaken type name "uint16_6" to the correct
"uint16_t" in the statement that computes chunkSize (the line casting
(endAddress - address) to uint16_t); update the cast near the assignment to the
variable chunkSize in tComm.c (where chunkSize, endAddress and address are used)
so the code compiles cleanly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Discovery/Src/tComm.c
🧰 Additional context used
🧬 Code graph analysis (1)
Discovery/Src/tComm.c (1)
Common/Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_uart.c (2)
HAL_UART_Receive(696-774)HAL_UART_Transmit(614-685)
🪛 GitHub Actions: Build the OSTC 4/5 Firmware
Discovery/Src/tComm.c
[error] 733-733: uint16_6 undeclared (first use in this function); did you mean 'uint16_t'?
🔇 Additional comments (3)
Discovery/Src/tComm.c (3)
662-663: LGTM! Exit condition correctly permits command 0x20.The refined condition properly allows command 0x20 to proceed while maintaining the original behavior for all other commands below 0x60.
668-668: LGTM! Helpful maintenance comment added.This comment usefully reminds developers to update the exit condition filter when adding support for additional commands in the restricted range.
675-675: LGTM! Command 0x20 correctly added to acknowledgment list.The placement is appropriate and consistent with the existing command handling pattern.
| while (address < endAddress) { | ||
| uint16_t chunkSize = 0x8000; | ||
| if ((endAddress - address) < 0x8000) { | ||
| chunkSize = (uint16_6)(endAddress - address); |
There was a problem hiding this comment.
CRITICAL: Fix compilation error - typo in type name.
Line 733 contains uint16_6 which should be uint16_t. This is causing a compilation failure as shown in the pipeline logs.
🔎 Proposed fix
- chunkSize = (uint16_6)(endAddress - address);
+ chunkSize = (uint16_t)(endAddress - address);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chunkSize = (uint16_6)(endAddress - address); | |
| chunkSize = (uint16_t)(endAddress - address); |
🧰 Tools
🪛 GitHub Actions: Build the OSTC 4/5 Firmware
[error] 733-733: uint16_6 undeclared (first use in this function); did you mean 'uint16_t'?
🤖 Prompt for AI Agents
In @Discovery/Src/tComm.c around line 733, The compilation error is from a typo
in the cast type on the assignment to chunkSize: change the mistaken type name
"uint16_6" to the correct "uint16_t" in the statement that computes chunkSize
(the line casting (endAddress - address) to uint16_t); update the cast near the
assignment to the variable chunkSize in tComm.c (where chunkSize, endAddress and
address are used) so the code compiles cleanly.
e27b0c6 to
fecf177
Compare
|
Artifacts: |
Made release build build cleanly. Updated build setup. Switched to using the original project files. Undo fix for linker script. Updated build configuration. Re-added printing of memory usage. Fixed warnings.
Fixed .gitignore.
Added reference to base upstream revision.
Fix building of firmware that does not contain all parts of (firmware, fonts, RTE). Added the option to only build parts to the un*x build script. Added external builder for Firmware and RTE. Fix RTE packaging. Added builder for fonts, updated packaging script to build Release (required for fonts). Added builders for packaging.
Signed-off-by: Michael Keller <github@ike.ch>
Signed-off-by: Michael Keller <github@ike.ch>
Signed-off-by: Michael Keller <github@ike.ch>
Signed-off-by: Michael Keller <github@ike.ch>
Signed-off-by: Michael Keller <github@ike.ch>
mikeller
force-pushed
the
development
branch
from
c2d826a to
7fec1ad
Compare
Signed-off-by: Michael Keller <github@ike.ch>
mikeller
force-pushed
the
add_memory_reading
branch
from
fecf177 to
1e565e4
Compare
|
Artifacts: |
mikeller
force-pushed
the
development
branch
2 times, most recently
from
198eea4 to
fa70217
Compare
Signed-off-by: Michael Keller github@ike.ch
Summary by CodeRabbit
New Features
Bug Fixes