Verify job profile propagation to bootloader services#109
Verify job profile propagation to bootloader services#109efargas merged 9 commits into011-bootloader-integrationfrom
Conversation
Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Latest suggestions up to 1d9e16c
Previous suggestions✅ Suggestions up to commit 8193234
✅ Suggestions up to commit 2dab2b2
|
|||||||||||||||||||||||||||||||||||||
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
tests/S7Tools.Tests/Services/Jobs/JobProfilePropagationTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
- Fix test class compilation error by moving constructor initialization code properly - Add InvariantCulture to uint.TryParse for hex parsing in EnhancedBootloaderService - Validate segment.Size is within uint range before casting to prevent overflow - Add EOF checks in PlcProtocolAdapter.ReceivePacketAsync to prevent infinite loops Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
@copilot apply this suggestions |
... All three code review suggestions have already been implemented in commit 60af664:
All 6 profile propagation tests pass successfully. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
| MemorySegment segment = selectedSegments[i]; | ||
| string start = segment.StartAddress ?? throw new InvalidOperationException("Memory segment start address is null."); | ||
|
|
||
| for (int i = 0; i < selectedSegments.Count; i++) | ||
| if (start.StartsWith("0x", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| MemorySegment segment = selectedSegments[i]; | ||
| uint segmentStart = uint.Parse(segment.StartAddress.Replace("0x", ""), System.Globalization.NumberStyles.HexNumber); | ||
| uint segmentSize = (uint)segment.Size; | ||
|
|
||
| progress.Report(("memory_dump", 0.50 + (0.45 * totalBytesRead / totalSize))); | ||
| _logger.LogDebug("Dumping segment {Index}/{Total}: '{Name}' @ 0x{Address:X8} ({Size} bytes)", | ||
| i + 1, selectedSegments.Count, segment.Name, segmentStart, segmentSize); | ||
|
|
||
| var segmentProgress = new Progress<long>(bytesRead => | ||
| { | ||
| double percent = 0.50 + (0.45 * (totalBytesRead + bytesRead) / totalSize); | ||
| progress.Report(("memory_dump", percent)); | ||
| }); | ||
|
|
||
| byte[] segmentData = await client.DumpMemoryAsync( | ||
| segmentStart, | ||
| segmentSize, | ||
| dumperPayload, | ||
| segmentProgress, | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| segmentDataList.Add(segmentData); | ||
| totalBytesRead += segmentData.Length; | ||
|
|
||
| _logger.LogInformation("Segment '{Name}' dumped successfully: {Size} bytes", segment.Name, segmentData.Length); | ||
| processLogger?.LogInformation("Segment {Index}/{Total} '{Name}': {Size} bytes from 0x{Start:X8}", | ||
| i + 1, selectedSegments.Count, segment.Name, segmentData.Length, segmentStart); | ||
| start = start[2..]; | ||
| } | ||
|
|
||
| // Concatenate all segment data | ||
| memoryData = segmentDataList.SelectMany(arr => arr).ToArray(); | ||
| _logger.LogInformation("Multi-segment dump completed: {TotalSegments} segments, {TotalSize} bytes total", | ||
| selectedSegments.Count, memoryData.Length); | ||
| } | ||
| else | ||
| { | ||
| // Single-region dump using legacy MemoryRegionProfile | ||
| progress.Report(("memory_dump", 0.50)); | ||
| _logger.LogDebug("Dumping memory region 0x{Address:X8} - 0x{EndAddress:X8} ({Length} bytes)", | ||
| profiles.Memory.Start, | ||
| profiles.Memory.Start + profiles.Memory.Length, | ||
| profiles.Memory.Length); | ||
|
|
||
| byte[] dumperPayload = await _payloads.GetMemoryDumperAsync( | ||
| profiles.Payloads.BasePath, | ||
| cancellationToken).ConfigureAwait(false); | ||
| if (!uint.TryParse( | ||
| start, | ||
| System.Globalization.NumberStyles.HexNumber, |
There was a problem hiding this comment.
Suggestion: Simplify hexadecimal address parsing logic
| MemorySegment segment = selectedSegments[i]; | |
| string start = segment.StartAddress ?? throw new InvalidOperationException("Memory segment start address is null."); | |
| for (int i = 0; i < selectedSegments.Count; i++) | |
| if (start.StartsWith("0x", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| MemorySegment segment = selectedSegments[i]; | |
| uint segmentStart = uint.Parse(segment.StartAddress.Replace("0x", ""), System.Globalization.NumberStyles.HexNumber); | |
| uint segmentSize = (uint)segment.Size; | |
| progress.Report(("memory_dump", 0.50 + (0.45 * totalBytesRead / totalSize))); | |
| _logger.LogDebug("Dumping segment {Index}/{Total}: '{Name}' @ 0x{Address:X8} ({Size} bytes)", | |
| i + 1, selectedSegments.Count, segment.Name, segmentStart, segmentSize); | |
| var segmentProgress = new Progress<long>(bytesRead => | |
| { | |
| double percent = 0.50 + (0.45 * (totalBytesRead + bytesRead) / totalSize); | |
| progress.Report(("memory_dump", percent)); | |
| }); | |
| byte[] segmentData = await client.DumpMemoryAsync( | |
| segmentStart, | |
| segmentSize, | |
| dumperPayload, | |
| segmentProgress, | |
| cancellationToken).ConfigureAwait(false); | |
| segmentDataList.Add(segmentData); | |
| totalBytesRead += segmentData.Length; | |
| _logger.LogInformation("Segment '{Name}' dumped successfully: {Size} bytes", segment.Name, segmentData.Length); | |
| processLogger?.LogInformation("Segment {Index}/{Total} '{Name}': {Size} bytes from 0x{Start:X8}", | |
| i + 1, selectedSegments.Count, segment.Name, segmentData.Length, segmentStart); | |
| start = start[2..]; | |
| } | |
| // Concatenate all segment data | |
| memoryData = segmentDataList.SelectMany(arr => arr).ToArray(); | |
| _logger.LogInformation("Multi-segment dump completed: {TotalSegments} segments, {TotalSize} bytes total", | |
| selectedSegments.Count, memoryData.Length); | |
| } | |
| else | |
| { | |
| // Single-region dump using legacy MemoryRegionProfile | |
| progress.Report(("memory_dump", 0.50)); | |
| _logger.LogDebug("Dumping memory region 0x{Address:X8} - 0x{EndAddress:X8} ({Length} bytes)", | |
| profiles.Memory.Start, | |
| profiles.Memory.Start + profiles.Memory.Length, | |
| profiles.Memory.Length); | |
| byte[] dumperPayload = await _payloads.GetMemoryDumperAsync( | |
| profiles.Payloads.BasePath, | |
| cancellationToken).ConfigureAwait(false); | |
| if (!uint.TryParse( | |
| start, | |
| System.Globalization.NumberStyles.HexNumber, | |
| MemorySegment segment = selectedSegments[i]; | |
| if (!uint.TryParse( | |
| segment.StartAddress, | |
| System.Globalization.NumberStyles.HexNumber, |
There was a problem hiding this comment.
@copilot review that suggestion and apply it if convenient, ensure all places where used and UI controls if needed to update accordingly
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Simplify and harden hexadecimal start address parsing by normalizing the segment start address (trim + optional 0x prefix removal) into a single variable before calling uint.TryParse, while keeping culture-invariant hex parsing and improving the invalid-address error message.
| MemorySegment segment = selectedSegments[i]; | |
| string start = segment.StartAddress ?? throw new InvalidOperationException("Memory segment start address is null."); | |
| for (int i = 0; i < selectedSegments.Count; i++) | |
| if (start.StartsWith("0x", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| MemorySegment segment = selectedSegments[i]; | |
| uint segmentStart = uint.Parse(segment.StartAddress.Replace("0x", ""), System.Globalization.NumberStyles.HexNumber); | |
| uint segmentSize = (uint)segment.Size; | |
| progress.Report(("memory_dump", 0.50 + (0.45 * totalBytesRead / totalSize))); | |
| _logger.LogDebug("Dumping segment {Index}/{Total}: '{Name}' @ 0x{Address:X8} ({Size} bytes)", | |
| i + 1, selectedSegments.Count, segment.Name, segmentStart, segmentSize); | |
| var segmentProgress = new Progress<long>(bytesRead => | |
| { | |
| double percent = 0.50 + (0.45 * (totalBytesRead + bytesRead) / totalSize); | |
| progress.Report(("memory_dump", percent)); | |
| }); | |
| byte[] segmentData = await client.DumpMemoryAsync( | |
| segmentStart, | |
| segmentSize, | |
| dumperPayload, | |
| segmentProgress, | |
| cancellationToken).ConfigureAwait(false); | |
| segmentDataList.Add(segmentData); | |
| totalBytesRead += segmentData.Length; | |
| _logger.LogInformation("Segment '{Name}' dumped successfully: {Size} bytes", segment.Name, segmentData.Length); | |
| processLogger?.LogInformation("Segment {Index}/{Total} '{Name}': {Size} bytes from 0x{Start:X8}", | |
| i + 1, selectedSegments.Count, segment.Name, segmentData.Length, segmentStart); | |
| start = start[2..]; | |
| } | |
| // Concatenate all segment data | |
| memoryData = segmentDataList.SelectMany(arr => arr).ToArray(); | |
| _logger.LogInformation("Multi-segment dump completed: {TotalSegments} segments, {TotalSize} bytes total", | |
| selectedSegments.Count, memoryData.Length); | |
| } | |
| else | |
| { | |
| // Single-region dump using legacy MemoryRegionProfile | |
| progress.Report(("memory_dump", 0.50)); | |
| _logger.LogDebug("Dumping memory region 0x{Address:X8} - 0x{EndAddress:X8} ({Length} bytes)", | |
| profiles.Memory.Start, | |
| profiles.Memory.Start + profiles.Memory.Length, | |
| profiles.Memory.Length); | |
| byte[] dumperPayload = await _payloads.GetMemoryDumperAsync( | |
| profiles.Payloads.BasePath, | |
| cancellationToken).ConfigureAwait(false); | |
| if (!uint.TryParse( | |
| start, | |
| System.Globalization.NumberStyles.HexNumber, | |
| MemorySegment segment = selectedSegments[i]; | |
| string rawStart = segment.StartAddress ?? throw new InvalidOperationException("Memory segment start address is null."); | |
| string start = rawStart.Trim(); | |
| if (start.StartsWith("0x", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| start = start[2..]; | |
| } | |
| if (!uint.TryParse( | |
| start, | |
| System.Globalization.NumberStyles.HexNumber, |
See review comment here
User description
Overview
Verification task confirming that task job profiles correctly propagate custom configurations to bootloader services without default value overwrites. Analysis shows the system is working as designed. Code review feedback addressed to fix compilation issues and improve robustness.
Changes Made
🧪 Test Suite (
tests/S7Tools.Tests/Services/Jobs/JobProfilePropagationTests.cs)Added 6 focused tests validating profile propagation:
PowerOnTimeMs,PowerOffDelayMs)All tests pass, confirming correct propagation.
Code Review Fixes:
_testDirectoryfield declaration and proper initialization sequence📋 Analysis Document (
PROFILE_PROPAGATION_ANALYSIS.md)Comprehensive architecture analysis documenting:
🔧 Robustness Improvements
EnhancedBootloaderService.cs:
CultureInfo.InvariantCulturetouint.TryParsefor culture-independent hex string parsingsegment.Sizeis within validuintrange (1..uint.MaxValue) before castingcheckedcast since overflow is now prevented by validationPlcProtocolAdapter.cs:
_transport.ReadAsynccalls to prevent infinite loops on EOFVerification Results
✅ Profile propagation working correctly:
✅ No default overwrites found:
profiles.PowerOnTimeMsandprofiles.PowerOffDelayMsdirectlyBreaking Changes
None. This is a verification task with robustness improvements. No functional changes to production code behavior.
Validation
Post-Merge Actions
None required. Verification confirms system is operating correctly with improved robustness.
Related: Addresses verification of profile propagation from job configuration through to bootloader execution, with additional robustness improvements based on code review feedback.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
PR Type
Tests, Documentation
Description
Add comprehensive test suite validating job profile propagation to bootloader services
Add detailed architecture analysis document confirming correct profile propagation flow
Fix code formatting inconsistencies across multiple service files
Diagram Walkthrough
File Walkthrough
JobProfilePropagationTests.cs
Add job profile propagation verification teststests/S7Tools.Tests/Services/Jobs/JobProfilePropagationTests.cs
propagation
PowerOffDelayMs) are preserved
preservation
immutable
PROFILE_PROPAGATION_ANALYSIS.md
Add profile propagation architecture analysis documentationPROFILE_PROPAGATION_ANALYSIS.md
propagation architecture
execution
enhancements
PlcMemoryManager.cs
Add braces to conditional statementsrc/S7Tools/Services/Adapters/Plc/PlcMemoryManager.cs
style consistency
PlcStagerManager.cs
Add braces to error handling conditionalsrc/S7Tools/Services/Adapters/Plc/PlcStagerManager.cs
handling
PlcProtocolAdapter.cs
Add braces to validation conditionalssrc/S7Tools/Services/Adapters/PlcProtocolAdapter.cs
EnhancedBootloaderService.cs
Fix indentation and whitespace formattingsrc/S7Tools/Services/Bootloader/EnhancedBootloaderService.cs
TaskDetailsViewModel.cs
Add braces to semaphore check conditionalssrc/S7Tools/ViewModels/Tasks/TaskDetailsViewModel.cs
async methods
ExecutePowerCycleAsync, ExecuteRunValidationAsync
PR Type
Enhancement, Tests
Description
Add comprehensive test suite validating job profile propagation to bootloader services
Implement robustness improvements for hex parsing and EOF handling in protocol adapters
Add validation for memory segment sizes to prevent overflow conditions
Fix code formatting and indentation issues across multiple files
Diagram Walkthrough
File Walkthrough
JobProfilePropagationTests.cs
New test suite for job profile propagation validationtests/S7Tools.Tests/Services/Jobs/JobProfilePropagationTests.cs
JobProfile to execution
PowerOffDelayMs)
preservation
remain immutable
conversion
PROFILE_PROPAGATION_ANALYSIS.md
Architecture analysis document for profile propagation verificationPROFILE_PROPAGATION_ANALYSIS.md
propagation flow
bootloader execution
detected
EnhancedBootloaderService.cs
Add hex parsing robustness and segment size validationsrc/S7Tools/Services/Bootloader/EnhancedBootloaderService.cs
CultureInfo.InvariantCulturetouint.TryParseforculture-independent hex string parsing
segment.Sizeis within validuintrange(1..uint.MaxValue) before casting
segment sizes
method
validation
PlcProtocolAdapter.cs
Add EOF detection and prevent infinite loops in protocol adaptersrc/S7Tools/Services/Adapters/PlcProtocolAdapter.cs
_transport.ReadAsynccalls to detect EOFconditions
InvalidOperationExceptionwhen EOF is reached while readingpacket length
data with byte count info
unexpectedly
PlcMemoryManager.cs
Fix code formatting in memory managersrc/S7Tools/Services/Adapters/Plc/PlcMemoryManager.cs
PlcStagerManager.cs
Fix code formatting in stager managersrc/S7Tools/Services/Adapters/Plc/PlcStagerManager.cs
TaskDetailsViewModel.cs
Fix code formatting in task details view modelsrc/S7Tools/ViewModels/Tasks/TaskDetailsViewModel.cs
ExecutePowerOffAsync, ExecutePowerCycleAsync, and
ExecuteRunValidationAsync methods
methods