Conversation
…s_interface into development # Conflicts: # README.md # canbus-core/src/main/java/io/mapsmessaging/canbus/device/InterfaceIndexResolver.java # canbus-core/src/main/java/io/mapsmessaging/canbus/device/JnaInterfaceIndexResolver.java # canbus-core/src/main/java/io/mapsmessaging/canbus/device/JnaLibCFacade.java # canbus-core/src/main/java/io/mapsmessaging/canbus/device/LibCFacade.java # pom.xml
…adding constants, validation, and utility methods
…rences, and add unit tests
… and validations accordingly
…variable naming, and optimize loop logic
…t loop logic for better test performance
… edge cases and input validations
…idation, add null check for `interfaceName`, and clean up unused code in `Vcan0ReadWriteDemo`
Stabilise SocketCAN interface resolution and improve CAN bus throughput handling
…and can1 interfaces
Canbus api fix
…okup and add range expansion logic
…e schema and fix redundant mock in `CanaerospaceFrameParserTest`
…e schema and fix redundant mock in `CanaerospaceFrameParserTest`
… serialization and add detailed unit tests for payload round-trips
…ers and frames to reduce allocation overhead
…ding from field value sources in processors Expanded `N2kMessageParser` to include `encodeFromSource` method and implemented `FieldValueSource`-based unpacking in `StringProcessor`, `NumericProcessor`, and others. Added unit tests for field value source handling.
…ith corresponding unit tests Introduced `SerialCanDevice` for CAN communication over serial interfaces. Added `SlcanCanFrameStreamCodec` and related classes, providing encoding/decoding for SLCAN-based CAN frames. Updated `CanFrame` with validation logic and added extensive unit tests for all new components.
Updated `CanFrame` to distinguish between classic and CAN FD frames with new constants and validation rules. Enhanced data length checks to support up to 64 bytes for CAN FD. Adjusted test to validate constraints for extended data lengths.
…nd ISO Address Claim PGNs Added detailed PGN configurations for `126993` (Heartbeat) and `60928` (ISO Address Claim) in the NMEA XML database, including field definitions and metadata.
…initions to NMEA database
…ield handling logic Introduced `StringLauProcessor` to handle `STRING_LAU` type fields, updated `N2kMessageParser` for dynamic field packing/unpacking, and refined payload length computations. Implemented new test cases for `STRING_FIX` and `STRING_LAU` fields.
…ding CAN frames Introduced a codec implementation for the Waveshare USB CAN adapter. Added logic for reading and writing CAN frames, including payload and identifier validation, extended frame support, and error handling for invalid packets.
Implemented extensive test cases to validate encoding and decoding of CAN frames, including standard and extended frames, payload handling, and error scenarios.
…vice` and `SerialCanDevice` to implement it
…ement classic CAN capabilities in codecs Introduced `getCanCapabilities` to the `CanDevice` interface for querying device capabilities. Implemented a default `classicCan` capability in `CanCapabilities` and integrated it with `SerialCanDevice` and `CanFrameStreamCodec`.
📝 WalkthroughWalkthroughThis pull request implements a comprehensive CAN bus abstraction layer with device/codec interfaces, adds extensive CANaerospace schema-driven message parsing capabilities, refactors NMEA2000 field processing to use cursor-based codec patterns, converts CAN frames to immutable records with validation, upgrades Java target to 21, and disables Maven SonarQube analysis. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CanaerospaceFrameParser
participant CanaerospaceSchemaRegistry
participant DataTypeCodec
participant ParsedCanaerospaceMessage
Client->>CanaerospaceSchemaRegistry: findIdentifier(canId)
CanaerospaceSchemaRegistry-->>CanaerospaceFrameParser: ResolvedIdentifier
Client->>CanaerospaceFrameParser: parse(canId, payload)
CanaerospaceFrameParser->>CanaerospaceFrameParser: Extract node/code/datatype fields
CanaerospaceFrameParser->>CanaerospaceSchemaRegistry: findMessageType(canId)
CanaerospaceSchemaRegistry-->>CanaerospaceFrameParser: MessageTypeDefinition
CanaerospaceFrameParser->>CanaerospaceSchemaRegistry: findDataTypeNameByNumber(number)
CanaerospaceSchemaRegistry-->>CanaerospaceFrameParser: dataTypeName
CanaerospaceFrameParser->>DataTypeCodec: decode(dataTypeName, bytes)
DataTypeCodec-->>CanaerospaceFrameParser: rawValue (typed)
CanaerospaceFrameParser->>CanaerospaceFrameParser: Compute engineeringValue = rawValue × resolution
CanaerospaceFrameParser->>ParsedCanaerospaceMessage: populate(canId, nodeId, rawValue, engineeringValue...)
ParsedCanaerospaceMessage-->>CanaerospaceFrameParser: initialized
CanaerospaceFrameParser-->>Client: ParsedCanaerospaceMessage
sequenceDiagram
participant Client
participant N2kMessageParser
participant Processor1 as NumericProcessor
participant Processor2 as StringLauProcessor
participant Processor3 as LookupProcessor
Client->>N2kMessageParser: encodeFromSource(pgn, FieldValueSource)
N2kMessageParser->>N2kMessageParser: Initialize cursor=0, payload[]
loop for each compiled field
N2kMessageParser->>Processor1: pack(field, payload, cursor, source)
Processor1->>Processor1: Read value from source via fieldId
Processor1->>Processor1: Scale/validate numeric value
Processor1->>Processor1: Insert bits at cursor position
Processor1-->>N2kMessageParser: newCursor
N2kMessageParser->>N2kMessageParser: cursor = newCursor
end
N2kMessageParser->>N2kMessageParser: Compute final payload length
N2kMessageParser-->>Client: byte[] payload (encoded)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| return 0; | ||
| } | ||
|
|
||
| Integer bitLength = field.getBitLength(); |
| } | ||
|
|
||
| private static int computeRelativeBytes(N2kCompiledField field) { | ||
| Integer bitLength = field.getBitLength(); |
| } | ||
|
|
||
| private static String buildFixedStringValue(N2kCompiledField field, String id) { | ||
| Integer bitLength = field.getBitLength(); |
| boolean reserved = fieldType == N2kFieldType.RESERVED; | ||
|
|
||
| String id = fieldDefinition.getId(); | ||
| if (!reserved && id == null || id.isBlank() ||!seenIds.add(id)) { |
| } | ||
|
|
||
| int bytesRead = libC.read(this.socketFileDescriptor, buffer, fdSize); | ||
| public CanFrame readFrame() throws IOException { |
| return canFrameStreamCodec.read(inputStream); | ||
| } | ||
|
|
||
| public void writeFrame(CanFrame canFrame) throws IOException { |
| this.canFrameStreamCodec.initialise(this.inputStream, this.outputStream); | ||
| } | ||
|
|
||
| public CanFrame readFrame() throws IOException { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/framing/FramePacker.java (1)
74-80:⚠️ Potential issue | 🟠 MajorHarden public
packFastPacketwith explicit precondition checks.After Line 74 made this method public, callers can now pass invalid inputs that previously were screened by
pack(...). Add guards forpayload == nullandpayload.length <= 8to avoid NPEs and invalid fast-packet framing.Proposed fix
public List<CanFrame> packFastPacket( int pgn, int canIdentifier, int sourceAddress, int destinationAddress, byte[] payload ) { + Objects.requireNonNull(payload, "payload"); + if (payload.length <= 8) { + throw new IllegalArgumentException("Fast packet requires payload length > 8 bytes"); + } + int sequenceId = sequenceProvider.nextSequenceId(pgn, sourceAddress, destinationAddress);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/framing/FramePacker.java` around lines 74 - 80, The public method packFastPacket currently lacks input validation and can receive null or too-short payloads now that it’s exposed; add explicit precondition checks at the start of packFastPacket (the method signature with parameters pgn, canIdentifier, sourceAddress, destinationAddress, byte[] payload) to throw an IllegalArgumentException (or NullPointerException for null if you prefer consistency) when payload == null or when payload.length <= 8, with clear messages (e.g., "payload must not be null" and "payload must be > 8 bytes for fast-packet framing") so the method no longer proceeds with invalid input and avoids NPEs or invalid framing.canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/framing/message/KnownMessage.java (1)
34-41:⚠️ Potential issue | 🟡 MinorInconsistency: field name
decodedvs error message"packet".The
Objects.requireNonNullerror message was changed to"packet"(line 40), but the field is still nameddecoded(line 34). If aNullPointerExceptionis thrown, the message will reference"packet"while the actual field isdecoded, which could confuse debugging.Either rename the field to
packetfor consistency with the PR-wide rename, or keep the error message as"decoded"to match the field name.Option 1: Keep error message consistent with field name
- this.decoded = Objects.requireNonNull(decoded, "packet"); + this.decoded = Objects.requireNonNull(decoded, "decoded");Option 2: Rename field to match new convention
- private final JsonObject decoded; + private final JsonObject packet; public KnownMessage(CanId canId, int rawCanIdentifier, byte[] payload, JsonObject decoded) { this.canId = Objects.requireNonNull(canId, "canId"); this.rawCanIdentifier = rawCanIdentifier; this.payload = Objects.requireNonNull(payload, "payload"); - this.decoded = Objects.requireNonNull(decoded, "packet"); + this.packet = Objects.requireNonNull(decoded, "packet"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/framing/message/KnownMessage.java` around lines 34 - 41, The constructor in KnownMessage uses Objects.requireNonNull(decoded, "packet") which mismatches the field name decoded; update either the error message or the field name for consistency: either change the requireNonNull call in the KnownMessage constructor to Objects.requireNonNull(decoded, "decoded") or rename the private final JsonObject decoded field to packet (and update all usages) so the constructor message and field name align.canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kJsonSchemaValidationTest.java (1)
202-204:⚠️ Potential issue | 🟡 MinorRemove debug print statement.
The
System.err.println("check")statement appears to be leftover debug code and should be removed before merging.🧹 Remove debug statement
if (fieldSchema.has("minimum")) { double min = fieldSchema.get("minimum").getAsDouble(); - if(value.getAsDouble() + tolerance < min){ - System.err.println("check"); - } assertTrue(value.getAsDouble() + tolerance >= min, "Field '" + fieldId + "' below minimum for PGN=" + pgn); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kJsonSchemaValidationTest.java` around lines 202 - 204, Remove the leftover debug print System.err.println("check") from the N2kJsonSchemaValidationTest code where the numeric range check uses value.getAsDouble() + tolerance < min; specifically, delete that statement inside the test method (the block referencing variables value, tolerance and min) so the test contains only the intended assertion or failure handling instead of printing to stderr.
🧹 Nitpick comments (21)
canbus-core/src/main/java/io/mapsmessaging/canbus/device/JnaInterfaceIndexResolver.java (1)
21-37: Make the unused socket parameter explicit to avoid API confusion.
socketFileDescriptoris now ignored. Consider renaming/documenting it as intentionally unused so callers don’t assume descriptor validity affects resolution.♻️ Proposed minimal clarity patch
- public int resolveInterfaceIndex(int socketFileDescriptor, String interfaceName) throws IOException { + public int resolveInterfaceIndex(int unusedSocketFileDescriptor, String interfaceName) throws IOException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-core/src/main/java/io/mapsmessaging/canbus/device/JnaInterfaceIndexResolver.java` around lines 21 - 37, The socketFileDescriptor parameter of resolveInterfaceIndex is unused and causes API confusion; update the method signature and/or documentation to make this explicit by renaming socketFileDescriptor to a clearly unused identifier (e.g., unusedSocketFileDescriptor or _socketFileDescriptor) and add a Javadoc line on resolveInterfaceIndex stating the descriptor is ignored, or alternatively annotate the parameter with `@SuppressWarnings`("unused") and a brief inline comment; ensure any callers are updated for the rename to avoid compilation errors.canbus-core/pi_setup.md (1)
90-118: Unify docs with the versioned systemd units to avoid config drift.Line 97 has users hand-create
canbus.service, while this PR also shipscan0.service/can1.service. Keeping both patterns without cross-reference will diverge over time.Suggested doc direction
Either:
- document installing the shipped unit files, or
- keep only the inline
canbus.serviceexample and remove/avoid duplicating separate unit definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-core/pi_setup.md` around lines 90 - 118, The docs currently instruct users to create a hand-written canbus.service while the repo also ships per-interface units can0.service and can1.service, which risks config drift; update the pi_setup.md section to either (A) replace the hand-created canbus.service example with instructions that install and enable the shipped can0.service and can1.service units (mentioning the unit names can0.service and can1.service and the enable/start commands), or (B) explicitly state that the example canbus.service is an alternative and add a cross-reference explaining the shipped per-interface units (can0.service/can1.service) so maintainers and users know which approach is recommended and how to install/enable the shipped units.canbus-J1939/src/main/java/io/mapsmessaging/canbus/j1939/CanIdBuilder.java (1)
45-53: Add explicit PGN range validation to prevent silent truncation.
pgnis accepted as anyint, but only the lower PGN bits are consumed. Rejecting out-of-range/negative values would avoid hidden caller bugs.♻️ Suggested guard
public class CanIdBuilder { private static final int EXTENDED_ID_MASK = 0x1FFFFFFF; private static final int PRIORITY_MASK = 0x07; private static final int BYTE_MASK = 0xFF; + private static final int PGN_MASK = 0x3FFFF; // 18-bit PGN private static final int PDU1_MAX_PF = 239; @@ public static int build(int pgn, int priority, int sourceAddress, int destinationAddress) { + if (pgn < 0 || (pgn & ~PGN_MASK) != 0) { + throw new IllegalArgumentException("pgn must be in range 0x00000..0x3FFFF, got 0x" + Integer.toHexString(pgn)); + } if (priority < 0 || priority > PRIORITY_MASK) { throw new IllegalArgumentException("priority must be in range 0..7, got " + priority); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-J1939/src/main/java/io/mapsmessaging/canbus/j1939/CanIdBuilder.java` around lines 45 - 53, The build(int pgn, int priority, int sourceAddress, int destinationAddress) method currently accepts any int for pgn but silently ignores bits outside the PGN field; add explicit validation at the start of build to reject negative values and values outside the valid J1939 PGN range (e.g., ensure pgn >= 0 && pgn <= MAX_PGN where MAX_PGN is defined to match the PGN bit-width used later), and throw an IllegalArgumentException with a clear message if invalid; reference the pgn parameter and constants/masks (dp, pf extraction and BYTE_MASK) when adding the check so the guard matches the bit operations already present.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NumericRange.java (1)
8-10: Add a range invariant guard (min <= max).
Line 9andLine 10allow reversed bounds with no fail-fast check. Consider validating this during schema load to prevent invalid metadata entering parsing paths.Suggested patch
`@Data` public class NumericRange { private BigDecimal min; private BigDecimal max; + + public void validate() { + if (min != null && max != null && min.compareTo(max) > 0) { + throw new IllegalArgumentException("NumericRange min must be <= max"); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NumericRange.java` around lines 8 - 10, The NumericRange class currently allows reversed bounds because min and max are stored without validation; add a fail-fast invariant check (min.compareTo(max) <= 0) when the range is created or when setters are used (e.g., in the NumericRange constructor or any factory method/withMin/withMax methods) and throw an IllegalArgumentException with a clear message if min > max so invalid metadata cannot be loaded.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/TimeSlotAllocationExample.java (1)
12-13: Consider validating interval and CAN ID domain constraints.
Line 12andLine 13would benefit from a fail-fast check (e.g., positive interval, valid CAN ID range) when schema data is loaded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/TimeSlotAllocationExample.java` around lines 12 - 13, Add fail-fast validation for TimeSlotAllocationExample by enforcing transmissionIntervalMs > 0 and a valid CAN ID range when schema data is loaded: implement a validate() method (or perform checks in the class constructor or setters setTransmissionIntervalMs and setCanId) that throws IllegalArgumentException if transmissionIntervalMs is null or <= 0 or if canId is null or outside the allowed CAN ID domain (e.g., 0..0x7FF for 11-bit standard IDs or your project’s configured range); call this validate() after deserialization/creation so invalid schema data fails fast.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/IdRange.java (1)
7-8: Consider validating CAN ID bounds and range ordering.
Line 7andLine 8currently accept any Integer values. Adding a guard for valid CAN ID bounds andmin <= maxwould reduce downstream parser ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/IdRange.java` around lines 7 - 8, IdRange currently allows any Integer for the fields min and max; add validation to enforce CAN ID bounds and ordering by checking non-null, 0 <= min <= max <= 0x1FFFFFFF (supporting extended 29-bit IDs) and throwing IllegalArgumentException when violated. Implement this check in the IdRange(Integer min, Integer max) constructor and in the setMin(Integer) and setMax(Integer) mutators (or a private validateRange() helper invoked by them), and ensure callers cannot create an invalid range by validating inputs before assignment.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServiceCodeDefinition.java (1)
8-9: Define precedence/one-of rule forserviceCodevsserviceCodeRange.Allowing both fields without an explicit exclusivity rule can create ambiguous resolution behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServiceCodeDefinition.java` around lines 8 - 9, NodeServiceCodeDefinition currently allows both serviceCode and serviceCodeRange to be set which creates ambiguity; update the class (NodeServiceCodeDefinition) to enforce a one-of rule by validating in the constructor/factory/builder or a validate() method that exactly one of serviceCode or serviceCodeRange is non-null (or define explicit precedence and document it), and throw an IllegalArgumentException (or similar) when the rule is violated; update any deserialization/creation paths to call this validation and add unit tests to cover both mutually-exclusive and precedence behaviors.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/FrameDefinition.java (1)
8-8: Use an enum-backed type or add validation forendiannessinstead of rawString.The field is declared as
Stringwith no validation or parsing logic. Since the codebase already usesByteOrderenum elsewhere, consider either:
- Converting
endiannessto useByteOrderenum directly, or- Adding runtime validation to constrain accepted string values (e.g., "BIG_ENDIAN", "LITTLE_ENDIAN") at schema load time.
This prevents typos and invalid configurations from silently propagating.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/FrameDefinition.java` at line 8, The endianness field in FrameDefinition is a raw String which risks invalid values; change it to use the existing ByteOrder enum (replace private String endianness with private ByteOrder endianness and update getters/setters and any deserialization logic to map strings to ByteOrder) or, if you must keep String, add validation in FrameDefinition (e.g., in the setter, constructor, or schema-load method) that parses the value against ByteOrder.valueOf(...) and throws a clear exception on invalid input; update any uses of getEndianness/setEndianness and JSON/XML deserialization code to handle the enum mapping.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/ResolvedCanDefinition.java (1)
23-23: Consider using@SingularfortemplateVariablesto enforce immutability.While
@Valuemakes the field reference final, the map contents remain mutable. Using Lombok's@Singular("templateVariable")annotation would wrap the map in an unmodifiable collection during construction, preventing accidental mutations. No mutation patterns were detected in the codebase, but this is a defensive best practice for preventing externally-introduced bugs.♻️ Suggested change
import lombok.Builder; +import lombok.Singular; import lombok.Value; @@ - Map<String, Object> templateVariables; + `@Singular`("templateVariable") + Map<String, Object> templateVariables;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/ResolvedCanDefinition.java` at line 23, The field templateVariables in ResolvedCanDefinition is currently a mutable Map despite the class being Lombok `@Value`; update the Lombok builder to use `@Singular`("templateVariable") on the templateVariables declaration so the generated builder wraps the map as an unmodifiable collection at construction time, ensuring immutability of the map contents; adjust any existing builder usages (ResolvedCanDefinition.builder()...) to continue working with the singular name if needed and remove any direct mutating logic (if present) that relies on a mutable map.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServicesDefinition.java (1)
5-13: InitializeserviceCodesto an empty list for consistency.
channelsandformatsare null-safe by default, butserviceCodesis not. Using an empty list here avoids extra null checks in parser/registry code paths.Proposed diff
import java.util.LinkedHashMap; +import java.util.ArrayList; import java.util.List; import java.util.Map; `@Data` public class NodeServicesDefinition { private Map<String, List<NodeServiceChannelDefinition>> channels = new LinkedHashMap<>(); - private List<NodeServiceCodeDefinition> serviceCodes; + private List<NodeServiceCodeDefinition> serviceCodes = new ArrayList<>(); private Map<String, NodeServiceFormatDefinition> formats = new LinkedHashMap<>(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServicesDefinition.java` around lines 5 - 13, NodeServicesDefinition currently leaves the field serviceCodes uninitialized which can cause NPEs; update the class so the field serviceCodes is initialized to an empty list (e.g., new ArrayList<>()) to match channels and formats null-safety. Modify the declaration of serviceCodes in NodeServicesDefinition to instantiate an empty List (import java.util.ArrayList if needed) so parser/registry code can avoid extra null checks.canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/parser/ParsedCanaerospaceMessage.java (1)
35-35: Defensively copydataBytesto avoid aliasing bugs.
byte[]is mutable; exposing it directly allows callers to mutate message state after parsing.♻️ Suggested change
`@Data` public class ParsedCanaerospaceMessage { @@ private byte[] dataBytes; @@ + public byte[] getDataBytes() { + return dataBytes == null ? null : dataBytes.clone(); + } + + public void setDataBytes(byte[] dataBytes) { + this.dataBytes = dataBytes == null ? null : dataBytes.clone(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/parser/ParsedCanaerospaceMessage.java` at line 35, ParsedCanaerospaceMessage exposes a mutable byte[] field dataBytes which can be aliased by callers; defensively copy it whenever accepting or returning the array to prevent external mutation. Update any constructor or setter that assigns to dataBytes (e.g., ParsedCanaerospaceMessage(...) or setDataBytes(...)) to assign a copy (clone or Arrays.copyOf) and update the getter (getDataBytes()) to return a copy instead of the internal array; ensure all internal assignments use copied arrays to avoid aliasing.canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/schema/N2kJsonSchemaGenerator.java (1)
48-51: Schema updated to use"packet"property — consider renaming local variable for clarity.The schema now correctly uses
"packet"as the property name (line 107) and requires it (line 113). However, the local variable is still nameddecoded(line 48), which is slightly confusing. Consider renaming it topacketfor consistency.Optional: Rename local variable for clarity
- JsonObject decoded = new JsonObject(); - decoded.addProperty("type", "object"); + JsonObject packet = new JsonObject(); + packet.addProperty("type", "object"); - JsonObject decodedProperties = new JsonObject(); + JsonObject packetProperties = new JsonObject(); // ... field processing ... - decoded.add("properties", decodedProperties); - decoded.addProperty("additionalProperties", false); + packet.add("properties", packetProperties); + packet.addProperty("additionalProperties", false); - properties.add("packet", decoded); + properties.add("packet", packet);Also applies to: 107-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/schema/N2kJsonSchemaGenerator.java` around lines 48 - 51, The local variable names used when building the JSON schema are inconsistent with the schema property "packet": rename the JsonObject variable currently named decoded to packet and decodedProperties to packetProperties in N2kJsonSchemaGenerator (where the schema sets "packet" as a property and requires it) so the variable names match the produced JSON structure; update all references within the method (e.g., where decoded.addProperty, decoded.add calls, and decodedProperties assignments are used) to the new names to keep code clear and consistent.canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java (1)
154-157: Remove unreachable null check onbitLength.
field.getBitLength()is primitive-backed (Lombok auto-generates a primitiveintgetter), making the null check at line 156 dead code.♻️ Proposed fix
- Integer bitLength = field.getBitLength(); - int lengthBytes = (bitLength == null || bitLength <= 0) ? 8 : (bitLength + 7) >>> 3; + int bitLength = field.getBitLength(); + int lengthBytes = (bitLength <= 0) ? 8 : (bitLength + 7) >>> 3;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java` around lines 154 - 157, The local variable bitLength should be a primitive int because N2kCompiledField.getBitLength() returns a primitive; remove the unreachable null check in buildFixedStringValue and compute lengthBytes directly from an int bitLength (e.g., int bitLength = field.getBitLength(); int lengthBytes = (bitLength <= 0) ? 8 : (bitLength + 7) >>> 3), replacing the Integer declaration and null check with this primitive-based logic.canbus-core/src/test/java/io/mapsmessaging/canbus/device/codec/SlcanCanFrameStreamCodecTest.java (1)
45-46: PreferassertTrue/assertFalseoverassertEqualsfor boolean assertions.Using
assertTrue(canFrame.extendedFrame())is more idiomatic thanassertEquals(true, canFrame.extendedFrame()). This pattern appears in multiple tests in this file.✨ Example fix
- assertEquals(true, canFrame.extendedFrame()); + assertTrue(canFrame.extendedFrame());Similarly for
assertEquals(false, ...):- assertEquals(false, canFrame.extendedFrame()); + assertFalse(canFrame.extendedFrame());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-core/src/test/java/io/mapsmessaging/canbus/device/codec/SlcanCanFrameStreamCodecTest.java` around lines 45 - 46, Replace boolean equality assertions using assertEquals with the idiomatic assertTrue/assertFalse in the tests; specifically, change assertions like assertEquals(true, canFrame.extendedFrame()) to assertTrue(canFrame.extendedFrame()) and assertEquals(false, someBoolean) to assertFalse(someBoolean). Update all occurrences in SlcanCanFrameStreamCodecTest that call canFrame.extendedFrame() (and any other boolean-returning methods) so they use junit.framework.Assert/Assertions' assertTrue/assertFalse API instead of assertEquals.canbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/CanFrameStreamCodec.java (1)
48-50: Clarify thatclose()does not close the streams.The
close()method only flushes the output stream but doesn't actually close the input/output streams. Consider adding a Javadoc comment to clarify this design decision, or renaming tocleanup()to avoid confusion with typicalclose()semantics.📝 Add clarifying documentation
+ /** + * Performs codec cleanup before stream closure. + * Note: This method flushes but does not close the streams; + * stream lifecycle is managed by the owning device. + */ default void close(InputStream inputStream, OutputStream outputStream) throws IOException { flush(outputStream); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/CanFrameStreamCodec.java` around lines 48 - 50, The close(InputStream inputStream, OutputStream outputStream) method currently only calls flush(outputStream) and does not close the streams, which is misleading; update the CanFrameStreamCodec API by either renaming close to a clearer name like cleanup or flushStreams, or add a Javadoc on the close(...) method that explicitly states it only flushes the OutputStream and does not close either stream; ensure the change references the existing close(InputStream, OutputStream) and flush(OutputStream) methods so callers know to close streams themselves or to use the new method name.canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/BaseTest.java (1)
211-293: Consider extracting shared logic between the twocomputeAllowedRawRangeoverloads.The
computeAllowedRawRange(N2kFieldDefinition)method (lines 228-293) shares significant logic withcomputeAllowedRawRange(N2kCompiledField)(lines 98-167). Consider extracting the common range computation into a private helper that accepts primitive parameters (bitLength,isSigned,rangeMin,rangeMax,resolution,offset) to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/BaseTest.java` around lines 211 - 293, The two methods computeAllowedRawRange(N2kFieldDefinition) and computeAllowedRawRange(N2kCompiledField) duplicate the same bit/scale clamping logic; extract that shared logic into a private helper (e.g., computeAllowedRawRangeForPrimitives or computeRawRange) that takes primitive parameters: int bitLength, boolean isSigned, Double rangeMin, Double rangeMax, double resolution, double offset and returns RawRange; then have both computeAllowedRawRange(...) overloads call that helper after converting field/compiledField properties (getBitLength(), isSigned(), getRangeMin()/getRangeMax(), getResolution(), getOffset()) and keep their existing early invalid/edge checks only if they differ in semantics.canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kJsonSchemaValidationTest.java (1)
145-159: Update stale comments referring to "decoded" instead of "packet".The comments on lines 148, 150, and 156 still reference "decoded" but the code now uses "packet" as the property name. Consider updating for consistency.
📝 Update comments
- assertTrue(envelope.has("packet"), "Missing decoded for PGN=" + pgn); + assertTrue(envelope.has("packet"), "Missing packet for PGN=" + pgn); JsonObject decoded = envelope.getAsJsonObject("packet"); - assertNotNull(decoded, "decoded is not an object for PGN=" + pgn); + assertNotNull(decoded, "packet is not an object for PGN=" + pgn); JsonObject schemaProperties = schema.getAsJsonObject("properties"); assertNotNull(schemaProperties, "Schema missing properties for PGN=" + pgn); JsonObject decodedSchema = schemaProperties.getAsJsonObject("packet"); - assertNotNull(decodedSchema, "Schema missing decoded for PGN=" + pgn); + assertNotNull(decodedSchema, "Schema missing packet for PGN=" + pgn);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kJsonSchemaValidationTest.java` around lines 145 - 159, Update the assertion error messages that still mention "decoded" to use "packet" to match the property name used in the code: change messages in the assertions around envelope.has/getAsJsonObject("packet") and the schema checks (references involving envelope, decoded, schemaProperties, decodedSchema, decodedSchemaProperties) so they say "Missing packet" / "packet is not an object" / "Schema missing packet" / "Schema packet missing properties" (preserve PGN context and existing variables names like envelope, decoded, schemaProperties, decodedSchema, decodedSchemaProperties).canbus-core/src/test/java/io/mapsmessaging/canbus/app/Vcan0ReadWriteDemo.java (1)
71-87: Consider logging or using the received frame for demo purposes.The
readFrame()result is discarded (line 73). For a demo utility, it might be helpful to log a sample of received frames periodically to show the system is working correctly.📝 Example enhancement
try { - reader.readFrame(); + CanFrame frame = reader.readFrame(); received++; Instant now = Instant.now(); if (Duration.between(lastPrint, now).toSeconds() >= 5) { - System.out.println("RX stats: received=" + received); + System.out.println("RX stats: received=" + received + ", last frame id=0x" + + Integer.toHexString(frame.canIdentifier())); lastPrint = now; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-core/src/test/java/io/mapsmessaging/canbus/app/Vcan0ReadWriteDemo.java` around lines 71 - 87, The demo discards the result of reader.readFrame(), so capture its return value (e.g., assign to a variable like frame = reader.readFrame()) and use it for periodic logging; update the loop in Vcan0ReadWriteDemo to increment received as before but also store the most recent frame and include a brief summary (timestamp, id, payload length or hex snippet) when printing RX stats (the same block that uses received and lastPrint), and ensure you still handle IOException and stop running via running.set(false).canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/compile/N2kCompiler.java (1)
147-159: Avoid callingbuilder.build()twice.
builder.build()is called on line 147-148 to accessgetBitLength(), and again on line 159 to add the field to the list. This creates an unnecessary intermediate object.♻️ Store bitLength in a local variable instead
if (compileTimeFixed && fieldDefinition.getBitOffset() != null && supportsBitStorage(fieldType)) { int bitOffset = fieldDefinition.getBitOffset(); int startByte = bitOffset >>> 3; int startBit = bitOffset & 7; + Integer bitLength = resolveBitLength(fieldDefinition); - if ( builder.build().getBitLength() > 0) { - int totalBits = startBit + builder.build().getBitLength(); + if (bitLength != null && bitLength > 0) { + int totalBits = startBit + bitLength; int bytesToRead = (totalBits + 7) >>> 3; builder .bitOffset(bitOffset) .startByte(startByte) .startBit(startBit) .bytesToRead(bytesToRead); } } compiledFields.add(builder.build());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/compile/N2kCompiler.java` around lines 147 - 159, The code currently calls builder.build() twice; instead call builder.build().getBitLength() once and store it in a local int (e.g., int bitLength = builder.build().getBitLength()), use that bitLength to compute totalBits and bytesToRead, set builder.bitOffset(...).startByte(...).startBit(...).bytesToRead(bytesToRead) and then call builder.build() a single time when adding to compiledFields (compiledFields.add(builder.build())); this avoids the duplicate build() invocation while keeping references to bitOffset, startByte, startBit, and bytesToRead intact.canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/Processor.java (1)
40-42: DefaultcomputePayloadLength(FieldValueSource)returns 0 - ensure processors override when needed.The default implementation returns 0, which is a no-op. Processors handling variable-length fields (like
StringLauProcessor) must override this to compute the correct payload length from the source. TheStringLauProcessorcorrectly overrides this (lines 26-34 in that file), but this could be a pitfall for future implementers.Consider adding a Javadoc comment to document that processors with variable-length fields must override this method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/Processor.java` around lines 40 - 42, Add a Javadoc to the Processor.computePayloadLength(N2kCompiledField, FieldValueSource) method explaining that the default implementation returns 0 and is a no-op, and that any Processor which handles variable-length fields (e.g., StringLauProcessor) must override computePayloadLength to compute the actual payload length from the FieldValueSource; reference the method name computePayloadLength, the Processor interface/class, FieldValueSource, and mention StringLauProcessor as an example so future implementers know to override it.canbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/SlcanCanFrameStreamCodec.java (1)
44-47: Recursive call for blank lines may cause stack overflow.Deeply nested recursion when processing many consecutive blank lines could exhaust the stack. Consider using a loop instead.
♻️ Suggested fix using a loop
- String trimmed = line.trim(); - if (trimmed.isEmpty()) { - return read(inputStream); - } + String trimmed = line.trim(); + while (trimmed.isEmpty()) { + line = readLine(inputStream); + if (line == null) { + return null; + } + trimmed = line.trim(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@canbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/SlcanCanFrameStreamCodec.java` around lines 44 - 47, The read(inputStream) method in SlcanCanFrameStreamCodec currently recurses when encountering blank lines (String trimmed = line.trim(); if (trimmed.isEmpty()) { return read(inputStream); }), risking stack overflow; replace the recursive retry with an iterative loop that reads lines from the same inputStream until a non-empty trimmed line is found (or EOF), then proceed with the existing parsing logic so no additional recursion occurs and blank-line handling is done in-place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7c0dfee-5bb8-4117-aec5-c3c09a6b702b
📒 Files selected for processing (86)
.buildkite/pipeline.ymlREADME.mdcanbus-J1939/src/main/java/io/mapsmessaging/canbus/j1939/CanId.javacanbus-J1939/src/main/java/io/mapsmessaging/canbus/j1939/CanIdBuilder.javacanbus-J1939/src/test/java/io/mapsmessaging/canbus/j1939/CanIdBuilderTest.javacanbus-core/pi_setup.mdcanbus-core/src/main/java/io/mapsmessaging/canbus/device/CanCapabilities.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/CanDevice.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/JnaInterfaceIndexResolver.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/JnaLibCFacade.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/LibCFacade.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/SerialCanDevice.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/SocketCanDevice.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/CanFrameStreamCodec.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/SlcanBitRate.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/SlcanCanFrameStreamCodec.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/impl/WaveshareUsbCanAStreamCodec.javacanbus-core/src/main/java/io/mapsmessaging/canbus/device/frames/CanFrame.javacanbus-core/src/main/script/can0.servicecanbus-core/src/main/script/can1.servicecanbus-core/src/test/java/io/mapsmessaging/canbus/app/CanLoad.javacanbus-core/src/test/java/io/mapsmessaging/canbus/app/Vcan0ReadWriteDemo.javacanbus-core/src/test/java/io/mapsmessaging/canbus/device/JnaInterfaceIndexResolverTest.javacanbus-core/src/test/java/io/mapsmessaging/canbus/device/SocketCanDeviceTest.javacanbus-core/src/test/java/io/mapsmessaging/canbus/device/codec/SlcanCanFrameStreamCodecTest.javacanbus-core/src/test/java/io/mapsmessaging/canbus/device/codec/WaveshareUsbCanAStreamCodecTest.javacanbus-core/src/test/java/io/mapsmessaging/canbus/device/frames/CanFrameTest.javacanbus-events/pom.xmlcanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/parser/CanaerospaceFrameParser.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/parser/DataTypeCodec.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/parser/ParsedCanaerospaceMessage.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/parser/ResolvedIdentifier.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/CanaerospaceSchema.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/CanaerospaceSchemaRegistry.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/DataTypeEntry.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/DataTypesDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/FrameDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/FrameDlcDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/HexRange.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/IdRange.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/IdentifierDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/IdentifierRangeDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/MessageTypeDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NativeFormatChannel.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeIdExample.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServiceChannelDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServiceCodeDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServiceFormatDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServiceMessageFormat.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NodeServicesDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NumericRange.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/NumericRangeInteger.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/PayloadFieldDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/ResolvedCanDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/SchemaMeta.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/ServiceCodeDefinition.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/TimeSlotAllocationExample.javacanbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/schema/ValidationDefinition.javacanbus-events/src/main/resources/canaerospace_schema.yamlcanbus-events/src/test/java/io/mapsmessaging/canbus/canaerospace/parser/CanaerospaceFrameParserTest.javacanbus-events/src/test/java/io/mapsmessaging/canbus/canaerospace/parser/CanaerospaceSchemaRoundTripParserTest.javacanbus-events/src/test/java/io/mapsmessaging/canbus/canaerospace/schema/CanaerospaceSchemaRegistryTest.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/FieldValueSource.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/LookupProcessor.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/N2kMessageParser.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/NumericProcessor.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/Processor.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/ReservedProcessor.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/StringLauProcessor.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/StringProcessor.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/compile/N2kCompiledField.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/compile/N2kCompiler.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/framing/FramePacker.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/framing/message/KnownMessage.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/schema/N2kJsonSchemaGenerator.javacanbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/schema/N2kSchemaRegistry.javacanbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xmlcanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/BaseTest.javacanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kCodecRoundTripTest.javacanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kJsonSchemaValidationTest.javacanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.javacanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripCanboatLogFramingTest.javacanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripCanboatLogTest.javacanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/codec/N2kMessageParserFieldValueSourceTest.javacanbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/schema/N2kJsonSchemaGeneratorTest.javapom.xml
💤 Files with no reviewable changes (1)
- .buildkite/pipeline.yml
|
|
||
| Example using **500 kbit/s**: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
Resolve markdownlint warnings (MD003/MD040) before release docs freeze.
Static analysis flags heading style inconsistency at Line 76 and fenced blocks without language identifiers in this section.
Suggested cleanup
- Convert the non-ATX heading at Line 76 to
##/###style. - Add explicit languages (for example
bash/ini) to the flagged fenced code blocks.
Also applies to: 95-135
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 76-76: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@canbus-core/pi_setup.md` at line 76, Convert the non-ATX heading in the
affected markdown section to an ATX-style heading (use ## or ###) and add
explicit language identifiers to all fenced code blocks (e.g., ```bash, ```ini)
in the same section; apply the same fixes to the subsequent fenced blocks in
that section so MD003/MD040 warnings are resolved.
Comment on lines
+110
to
+115
| ExecStart=/sbin/ip link set can0 up type can bitrate 500000 | ||
| ExecStart=/sbin/ip link set can1 up type can bitrate 500000 | ||
|
|
||
| ExecStop=/sbin/ip link set can0 down | ||
| ExecStop=/sbin/ip link set can1 down | ||
|
|
There was a problem hiding this comment.
Align systemd example runtime options with the shipped services.
Lines 110–115 omit restart-ms and txqueuelen used in can0.service/can1.service, so behavior differs between “doc path” and “artifact path.”
💡 Suggested edit
-ExecStart=/sbin/ip link set can0 up type can bitrate 500000
-ExecStart=/sbin/ip link set can1 up type can bitrate 500000
+ExecStart=/sbin/ip link set can0 down
+ExecStart=/sbin/ip link set can0 up type can bitrate 500000 restart-ms 100
+ExecStart=/sbin/ip link set can0 txqueuelen 5000
+ExecStart=/sbin/ip link set can1 down
+ExecStart=/sbin/ip link set can1 up type can bitrate 500000 restart-ms 100
+ExecStart=/sbin/ip link set can1 txqueuelen 5000📝 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.
Suggested change
| ExecStart=/sbin/ip link set can0 up type can bitrate 500000 | |
| ExecStart=/sbin/ip link set can1 up type can bitrate 500000 | |
| ExecStop=/sbin/ip link set can0 down | |
| ExecStop=/sbin/ip link set can1 down | |
| ExecStart=/sbin/ip link set can0 down | |
| ExecStart=/sbin/ip link set can0 up type can bitrate 500000 restart-ms 100 | |
| ExecStart=/sbin/ip link set can0 txqueuelen 5000 | |
| ExecStart=/sbin/ip link set can1 down | |
| ExecStart=/sbin/ip link set can1 up type can bitrate 500000 restart-ms 100 | |
| ExecStart=/sbin/ip link set can1 txqueuelen 5000 | |
| ExecStop=/sbin/ip link set can0 down | |
| ExecStop=/sbin/ip link set can1 down |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@canbus-core/pi_setup.md` around lines 110 - 115, The example systemd
ExecStart lines in pi_setup.md (the ExecStart entries that bring up can0 and
can1) are missing the same runtime options used in the shipped
can0.service/can1.service (restart-ms and txqueuelen); update the ExecStart
commands in pi_setup.md to include the same restart-ms=<value> and
txqueuelen=<value> options used by can0.service/can1.service so the documented
behavior matches the shipped services.
Comment on lines
+70
to
+86
| int canIdentifier = Integer.parseUnsignedInt(trimmed.substring(1, 1 + identifierLength), 16); | ||
| int dataLengthCode = Character.digit(trimmed.charAt(1 + identifierLength), 16); | ||
| if (dataLengthCode < 0 || dataLengthCode > 8) { | ||
| throw new IOException("Invalid SLCAN DLC: " + trimmed); | ||
| } | ||
|
|
||
| int expectedLength = 1 + identifierLength + 1 + (dataLengthCode * 2); | ||
| if (trimmed.length() != expectedLength) { | ||
| throw new IOException("Invalid SLCAN frame length: " + trimmed); | ||
| } | ||
|
|
||
| byte[] data = new byte[dataLengthCode]; | ||
| int offset = 1 + identifierLength + 1; | ||
| for (int index = 0; index < dataLengthCode; index++) { | ||
| int start = offset + (index * 2); | ||
| data[index] = (byte) Integer.parseInt(trimmed.substring(start, start + 2), 16); | ||
| } |
There was a problem hiding this comment.
Uncaught NumberFormatException for malformed hex input.
Integer.parseUnsignedInt and Integer.parseInt throw NumberFormatException for invalid hex strings, which would propagate as an unchecked exception rather than the expected IOException.
🐛 Suggested fix to wrap hex parsing exceptions
- int canIdentifier = Integer.parseUnsignedInt(trimmed.substring(1, 1 + identifierLength), 16);
+ int canIdentifier;
+ try {
+ canIdentifier = Integer.parseUnsignedInt(trimmed.substring(1, 1 + identifierLength), 16);
+ } catch (NumberFormatException e) {
+ throw new IOException("Invalid SLCAN identifier: " + trimmed, e);
+ }
int dataLengthCode = Character.digit(trimmed.charAt(1 + identifierLength), 16);
if (dataLengthCode < 0 || dataLengthCode > 8) {
throw new IOException("Invalid SLCAN DLC: " + trimmed);
}
int expectedLength = 1 + identifierLength + 1 + (dataLengthCode * 2);
if (trimmed.length() != expectedLength) {
throw new IOException("Invalid SLCAN frame length: " + trimmed);
}
byte[] data = new byte[dataLengthCode];
int offset = 1 + identifierLength + 1;
for (int index = 0; index < dataLengthCode; index++) {
int start = offset + (index * 2);
- data[index] = (byte) Integer.parseInt(trimmed.substring(start, start + 2), 16);
+ try {
+ data[index] = (byte) Integer.parseInt(trimmed.substring(start, start + 2), 16);
+ } catch (NumberFormatException e) {
+ throw new IOException("Invalid SLCAN data byte at index " + index + ": " + trimmed, e);
+ }
}📝 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.
Suggested change
| int canIdentifier = Integer.parseUnsignedInt(trimmed.substring(1, 1 + identifierLength), 16); | |
| int dataLengthCode = Character.digit(trimmed.charAt(1 + identifierLength), 16); | |
| if (dataLengthCode < 0 || dataLengthCode > 8) { | |
| throw new IOException("Invalid SLCAN DLC: " + trimmed); | |
| } | |
| int expectedLength = 1 + identifierLength + 1 + (dataLengthCode * 2); | |
| if (trimmed.length() != expectedLength) { | |
| throw new IOException("Invalid SLCAN frame length: " + trimmed); | |
| } | |
| byte[] data = new byte[dataLengthCode]; | |
| int offset = 1 + identifierLength + 1; | |
| for (int index = 0; index < dataLengthCode; index++) { | |
| int start = offset + (index * 2); | |
| data[index] = (byte) Integer.parseInt(trimmed.substring(start, start + 2), 16); | |
| } | |
| int canIdentifier; | |
| try { | |
| canIdentifier = Integer.parseUnsignedInt(trimmed.substring(1, 1 + identifierLength), 16); | |
| } catch (NumberFormatException e) { | |
| throw new IOException("Invalid SLCAN identifier: " + trimmed, e); | |
| } | |
| int dataLengthCode = Character.digit(trimmed.charAt(1 + identifierLength), 16); | |
| if (dataLengthCode < 0 || dataLengthCode > 8) { | |
| throw new IOException("Invalid SLCAN DLC: " + trimmed); | |
| } | |
| int expectedLength = 1 + identifierLength + 1 + (dataLengthCode * 2); | |
| if (trimmed.length() != expectedLength) { | |
| throw new IOException("Invalid SLCAN frame length: " + trimmed); | |
| } | |
| byte[] data = new byte[dataLengthCode]; | |
| int offset = 1 + identifierLength + 1; | |
| for (int index = 0; index < dataLengthCode; index++) { | |
| int start = offset + (index * 2); | |
| try { | |
| data[index] = (byte) Integer.parseInt(trimmed.substring(start, start + 2), 16); | |
| } catch (NumberFormatException e) { | |
| throw new IOException("Invalid SLCAN data byte at index " + index + ": " + trimmed, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@canbus-core/src/main/java/io/mapsmessaging/canbus/device/codec/SlcanCanFrameStreamCodec.java`
around lines 70 - 86, The hex parsing calls in SlcanCanFrameStreamCodec (the
Integer.parseUnsignedInt for the CAN identifier and the Integer.parseInt inside
the data byte loop) can throw NumberFormatException and should be caught and
rethrown as IOException; wrap the identifier parse and the per-byte parse in
try/catch blocks that catch NumberFormatException and throw new IOException with
a clear message including the offending trimmed string (and index for the byte
loop) so callers receive a checked IOException instead of an unchecked
NumberFormatException.
Comment on lines
+163
to
+169
| @Test | ||
| void constructor_rejectsInvalidDataLengthCode() { | ||
| Assertions.assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> new CanFrame(0x123, false, 9, new byte[8]) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test name is misleading — this fails on data length, not DLC validation.
The test name suggests it validates DLC bounds, but with dataLengthCode=9 and data.length=8, the exception is thrown by validateData (because data.length < dataLengthCode) rather than validateDataLengthCode (which allows 0-64). The test passes, but the reason differs from what the name implies.
Consider either:
- Renaming to
constructor_rejectsDataShorterThanDlc_classic(but that duplicates line 179-185), or - Testing actual DLC bounds:
new CanFrame(0x123, false, 65, new byte[65])to exceed the 64-byte limit.
Suggested fix to test actual DLC validation
`@Test`
- void constructor_rejectsInvalidDataLengthCode() {
+ void constructor_rejectsDataLengthCodeAbove64() {
Assertions.assertThrows(
IllegalArgumentException.class,
- () -> new CanFrame(0x123, false, 9, new byte[8])
+ () -> new CanFrame(0x123, false, 65, new byte[65])
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@canbus-core/src/test/java/io/mapsmessaging/canbus/device/frames/CanFrameTest.java`
around lines 163 - 169, The test name is misleading because the thrown exception
comes from validateData (data.length < dataLengthCode) not
validateDataLengthCode; update the test in CanFrameTest so it actually validates
DLC bounds by calling new CanFrame(0x123, false, 65, new byte[65]) to trigger
the 0-64 check in validateDataLengthCode, or alternatively rename the test to
reflect the real failure (e.g., constructor_rejectsDataShorterThanDlc) and keep
the existing call that exercises validateData; reference CanFrame, validateData,
and validateDataLengthCode when making the change.
Comment on lines
+47
to
+67
| if (resolvedIdentifier != null && resolvedIdentifier.getDataType() != null) { | ||
| expectedDataTypeName = resolvedIdentifier.getDataType(); | ||
|
|
||
| raw = DataTypeCodec.decode(expectedDataTypeName, dataBytes); | ||
|
|
||
| Double resolution = resolvedIdentifier.getResolution(); | ||
| if (resolution != null && raw instanceof Number number) { | ||
| engineeringValue = number.doubleValue() * resolution; | ||
| } else if (raw instanceof Float floatValue) { | ||
| engineeringValue = floatValue.doubleValue(); | ||
| } else if (raw instanceof Double doubleValue) { | ||
| engineeringValue =doubleValue; | ||
| } | ||
| } | ||
|
|
||
| Optional<String> payloadDataTypeNameOptional = registry.findDataTypeNameByNumber(payloadDataTypeNumber); | ||
| String payloadDataTypeName = payloadDataTypeNameOptional.orElse(null); | ||
|
|
||
| boolean dataTypeMismatch = false; | ||
| if (expectedDataTypeName != null && payloadDataTypeName != null) { | ||
| dataTypeMismatch = !expectedDataTypeName.equals(payloadDataTypeName); |
There was a problem hiding this comment.
Decode using the payload’s declared datatype before checking for mismatches.
payload[1] is the wire datatype, but this method decodes rawValue with resolvedIdentifier.getDataType() and only afterwards sets dataTypeMismatch. When the payload type differs from the schema type, callers get a decoded value produced by the wrong codec.
Suggested direction
- Object raw = null;
- Double engineeringValue = null;
- String expectedDataTypeName = null;
+ Object raw = null;
+ Double engineeringValue = null;
+ String expectedDataTypeName = null;
+ String payloadDataTypeName = registry.findDataTypeNameByNumber(payloadDataTypeNumber).orElse(null);
if (resolvedIdentifier != null && resolvedIdentifier.getDataType() != null) {
expectedDataTypeName = resolvedIdentifier.getDataType();
-
- raw = DataTypeCodec.decode(expectedDataTypeName, dataBytes);
+ String decodeTypeName = payloadDataTypeName != null ? payloadDataTypeName : expectedDataTypeName;
+ raw = decodeTypeName == null ? null : DataTypeCodec.decode(decodeTypeName, dataBytes);
...
}
-
- Optional<String> payloadDataTypeNameOptional = registry.findDataTypeNameByNumber(payloadDataTypeNumber);
- String payloadDataTypeName = payloadDataTypeNameOptional.orElse(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@canbus-events/src/main/java/io/mapsmessaging/canbus/canaerospace/parser/CanaerospaceFrameParser.java`
around lines 47 - 67, The code decodes using resolvedIdentifier.getDataType()
before checking the payload's declared datatype, so when payload[1] differs from
the schema you end up decoding with the wrong codec; change the flow to first
resolve payloadDataTypeName via
registry.findDataTypeNameByNumber(payloadDataTypeNumber) and use that (if
present) as the datatype passed to DataTypeCodec.decode, falling back to
resolvedIdentifier.getDataType() only when the payload type is null, then set
dataTypeMismatch by comparing expectedDataTypeName (from resolvedIdentifier) to
payloadDataTypeName, and finally apply resolution from resolvedIdentifier (if
any) to numeric raw to compute engineeringValue (so keep handling of resolution
and numeric conversions using raw, engineeringValue,
resolvedIdentifier.getResolution(), and DataTypeCodec.decode).
Comment on lines
+9
to
+31
| public int pack(N2kCompiledField field, byte[] payload, int cursor, JsonObject decoded) { | ||
| if (!decoded.has(field.getId()) || decoded.get(field.getId()).isJsonNull()) { | ||
| return cursor; | ||
| } | ||
|
|
||
| double numericValue = decoded.get(field.getId()).getAsDouble(); | ||
| return writeNumericValue(field, payload, cursor, numericValue); | ||
| } | ||
|
|
||
| @Override | ||
| public int pack(N2kCompiledField field, byte[] payload, int cursor, FieldValueSource source) { | ||
| Double numericValue = source.getDouble(field.getId()); | ||
| if (numericValue != null) { | ||
| return writeNumericValue(field, payload, cursor, numericValue); | ||
| } | ||
|
|
||
| Long longValue = source.getLong(field.getId()); | ||
| if (longValue != null) { | ||
| return writeNumericValue(field, payload, cursor, longValue.doubleValue()); | ||
| } | ||
|
|
||
| return cursor; | ||
| } |
There was a problem hiding this comment.
Advance the cursor when a cursor-relative numeric field is missing.
For non-fixed fields, the no-value path currently returns the incoming cursor. computePayloadLength(...) still allocates this field’s bytes, so every following cursor-based field gets packed into the wrong offset when the numeric value is omitted.
Suggested fix
public int pack(N2kCompiledField field, byte[] payload, int cursor, JsonObject decoded) {
if (!decoded.has(field.getId()) || decoded.get(field.getId()).isJsonNull()) {
- return cursor;
+ return field.isCompileTimeFixed() ? cursor : cursor + computeRelativeBytes(field);
}
double numericValue = decoded.get(field.getId()).getAsDouble();
return writeNumericValue(field, payload, cursor, numericValue);
}
@@
public int pack(N2kCompiledField field, byte[] payload, int cursor, FieldValueSource source) {
Double numericValue = source.getDouble(field.getId());
if (numericValue != null) {
return writeNumericValue(field, payload, cursor, numericValue);
}
Long longValue = source.getLong(field.getId());
if (longValue != null) {
return writeNumericValue(field, payload, cursor, longValue.doubleValue());
}
- return cursor;
+ return field.isCompileTimeFixed() ? cursor : cursor + computeRelativeBytes(field);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/codec/NumericProcessor.java`
around lines 9 - 31, When a numeric field is missing the pack methods currently
return the incoming cursor, but computePayloadLength(...) still reserves bytes
for non-fixed (cursor-relative) fields; update both pack(N2kCompiledField,
byte[], int, JsonObject) and pack(N2kCompiledField, byte[], int,
FieldValueSource) so that when a value is absent you advance the cursor by the
allocated size for that field (e.g., add cursor += computePayloadLength(field)
or cursor += field.getAllocatedByteLength()) for non-fixed/cursor-relative
fields instead of returning the original cursor. Ensure you only advance for the
no-value path and keep existing behavior when a value is written by
writeNumericValue(...).
Comment on lines
+103
to
+105
| if (!reserved && id == null || id.isBlank() ||!seenIds.add(id)) { | ||
| return isPositionFixed; | ||
| } |
There was a problem hiding this comment.
Fix operator precedence in the id validation condition.
The condition !reserved && id == null || id.isBlank() ||!seenIds.add(id) has unclear precedence. Due to && binding tighter than ||, this evaluates as ((!reserved && id == null) || id.isBlank() || !seenIds.add(id)).
If reserved is true, the first clause is false, but id.isBlank() or the duplicate check can still trigger a return. This might cause issues if id is null (NPE on id.isBlank()).
🔧 Proposed fix with explicit grouping and null-safety
- if (!reserved && id == null || id.isBlank() ||!seenIds.add(id)) {
+ if (!reserved && (id == null || id.isBlank() || !seenIds.add(id))) {
return isPositionFixed;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/compile/N2kCompiler.java`
around lines 103 - 105, The current compound condition in the N2kCompiler method
mixes && and || and can cause a NullPointerException when id is null; update the
guard to first check reserved and then group the id checks with null-safety so
id.isBlank() is only called when id != null. Concretely, change the condition
involving reserved, id, and seenIds (the current expression using reserved, id,
id.isBlank(), and seenIds.add(id)) to use explicit grouping and short-circuit
null checks (e.g., ensure id == null is evaluated before id.isBlank() and wrap
the three id-related checks together) so the return uses isPositionFixed only
when !reserved AND one of the grouped id conditions is true.
Comment on lines
+61
to
+70
| <Field> | ||
| <Order>2</Order> | ||
| <Id>reserved</Id> | ||
| <Name>Reserved</Name> | ||
| <BitLength>56</BitLength> | ||
| <BitOffset>8</BitOffset> | ||
| <BitStart>0</BitStart> | ||
| <FieldType>BINARY</FieldType> | ||
| <TypeInPdf>reserved</TypeInPdf> | ||
| </Field> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find FieldType enum/definitions"
rg -n --type=java 'enum\s+FieldType|class\s+.*FieldType'
echo
echo "2) Find parser/encoder handling for RESERVED/BINARY/NUMBER"
rg -n --type=java 'case\s+(RESERVED|BINARY|NUMBER)|FieldType\.(RESERVED|BINARY|NUMBER)'
echo
echo "3) Spot reserved-bit handling logic"
rg -n --type=java 'reserved|padding|skip|ignore'Repository: Maps-Messaging/canbus_interface
Length of output: 110
🏁 Script executed:
find . -name "NMEA_database_1_300.xml" -type fRepository: Maps-Messaging/canbus_interface
Length of output: 140
🏁 Script executed:
ls -la canbus-nmea2000/ 2>/dev/null | head -20 || find . -type f -name "*.xml" | head -20Repository: Maps-Messaging/canbus_interface
Length of output: 356
🏁 Script executed:
cat -n ./canbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xml | sed -n '55,75p'Repository: Maps-Messaging/canbus_interface
Length of output: 792
🏁 Script executed:
cat -n ./canbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xml | sed -n '148,170p'Repository: Maps-Messaging/canbus_interface
Length of output: 947
🏁 Script executed:
# Check all unique FieldType values in the file
rg -o '<FieldType>[^<]+</FieldType>' ./canbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xml | sort | uniq -c | sort -rnRepository: Maps-Messaging/canbus_interface
Length of output: 350
🏁 Script executed:
# Search for all reserved field definitions
rg -B2 -A10 '<Id>reserved</Id>' ./canbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xmlRepository: Maps-Messaging/canbus_interface
Length of output: 45545
Standardize reserved field types to RESERVED.
Lines 68 and 164 use inconsistent FieldType values for reserved bits: BINARY and NUMBER respectively. The schema defines 131 reserved fields as RESERVED type; only these 2 instances deviate. The NUMBER field at line 164 also carries extraneous properties (Resolution, Signed, RangeMin, RangeMax) inappropriate for reserved bits.
🔧 Proposed XML fixes
Line 68: Change <FieldType>BINARY</FieldType> to <FieldType>RESERVED</FieldType>
Lines 160–164: Replace with <FieldType>RESERVED</FieldType> (remove Resolution, Signed, RangeMin, RangeMax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@canbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xml` around lines
61 - 70, Two <Field> entries defined for reserved bits are using incorrect types
and extra attributes; locate the <Field> elements with Id "reserved" (one with
BitOffset 8/BitStart 0 and the other around the later reserved block) and change
their <FieldType> values to RESERVED and remove any non-applicable attributes
(Resolution, Signed, RangeMin, RangeMax) from the reserved entry so both
reserved fields match the schema's RESERVED type and contain no extraneous
properties.
Comment on lines
+101
to
+102
| envelope.addProperty("name", msg.getId()); | ||
| envelope.add("packet", decoded); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "N2kRoundTripAllPgnsTest.java" -type fRepository: Maps-Messaging/canbus_interface
Length of output: 170
🏁 Script executed:
cat -n ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java | sed -n '90,130p'Repository: Maps-Messaging/canbus_interface
Length of output: 1876
🏁 Script executed:
cat -n ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java | sed -n '1,50p'Repository: Maps-Messaging/canbus_interface
Length of output: 2341
🏁 Script executed:
cat -n ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java | sed -n '43,75p'Repository: Maps-Messaging/canbus_interface
Length of output: 1474
🏁 Script executed:
cat -n ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java | wc -lRepository: Maps-Messaging/canbus_interface
Length of output: 79
🏁 Script executed:
cat -n ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java | sed -n '100,115p'Repository: Maps-Messaging/canbus_interface
Length of output: 793
🏁 Script executed:
cat -n ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java | sed -n '48,56p'Repository: Maps-Messaging/canbus_interface
Length of output: 519
🏁 Script executed:
# Check if there are any other usages of msg.getId() to understand the contract
rg "getId()" ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java -A 2 -B 2Repository: Maps-Messaging/canbus_interface
Length of output: 759
🏁 Script executed:
# Let's check the N2kCompiledMessage class to understand the getId() contract
find . -name "N2kCompiledMessage.java" -type f | head -1Repository: Maps-Messaging/canbus_interface
Length of output: 173
🏁 Script executed:
cat -n ./canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/compile/N2kCompiledMessage.java | head -100Repository: Maps-Messaging/canbus_interface
Length of output: 1698
🏁 Script executed:
cat -n ./canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java | sed -n '59,75p'Repository: Maps-Messaging/canbus_interface
Length of output: 756
🏁 Script executed:
# Let's check if there are any messages in the registry with null IDs
# by looking at how messages are built
rg "getId()" ./canbus-nmea2000/src/main/java/io/mapsmessaging/canbus/j1939/n2k/compile/ -A 2 -B 2 | head -50Repository: Maps-Messaging/canbus_interface
Length of output: 2103
🏁 Script executed:
# Check if there are any null IDs in the actual message definitions
# by looking for PGNs with no ID
rg "messageDefinition\\.getId()" ./canbus-nmea2000/src/main/java -A 2 -B 2Repository: Maps-Messaging/canbus_interface
Length of output: 1453
🏁 Script executed:
# Let's look for the actual message definition files or data
find . -name "*.json" -o -name "*.xml" | grep -i "n2k\|pgn" | head -20Repository: Maps-Messaging/canbus_interface
Length of output: 140
🏁 Script executed:
# Check if there are any messages without an ID in the NMEA database
grep -o '<Message[^>]*>' ./canbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xml | head -20Repository: Maps-Messaging/canbus_interface
Length of output: 57
🏁 Script executed:
# Let's examine the structure of the XML file
head -100 ./canbus-nmea2000/src/main/resources/n2k/NMEA_database_1_300.xmlRepository: Maps-Messaging/canbus_interface
Length of output: 2895
Add null-safe handling for message ID in round-trip assertion.
At line 110, get("name").getAsString() throws IllegalStateException when msg.getId() is null (which can occur per line 53's null check). The test fails with a misleading exception instead of a proper assertion error. Guard the assertion to safely handle null IDs from the envelope round-trip.
Suggested fix
- envelope.addProperty("name", msg.getId());
+ if (msg.getId() == null || msg.getId().isBlank()) {
+ envelope.add("name", null);
+ } else {
+ envelope.addProperty("name", msg.getId());
+ }
- assertEquals(msg.getId(), decodedBackEnvelope.get("name").getAsString());
+ JsonElement nameElement = decodedBackEnvelope.get("name");
+ if (msg.getId() == null || msg.getId().isBlank()) {
+ assertTrue(nameElement == null || nameElement.isJsonNull());
+ } else {
+ assertNotNull(nameElement);
+ assertEquals(msg.getId(), nameElement.getAsString());
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@canbus-nmea2000/src/test/java/io/mapsmessaging/canbus/j1939/n2k/N2kRoundTripAllPgnsTest.java`
around lines 101 - 102, The test reads envelope.get("name").getAsString() which
throws when msg.getId() was null; update the round-trip assertion in
N2kRoundTripAllPgnsTest to be null-safe: after performing the round-trip, check
envelope.has("name") and !envelope.get("name").isJsonNull() before calling
getAsString() and assert equality with msg.getId(), otherwise
assertNull(msg.getId()) (or assertTrue/envelope check) so the assertion covers
both non-null IDs and null IDs produced by envelope.addProperty("name",
msg.getId()); reference the envelope object and msg.getId() usage and replace
the direct get("name").getAsString() call with this guarded check.
| <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding> | ||
|
|
||
| <maven.compiler.release>17</maven.compiler.release> | ||
| <maven.compiler.release>21</maven.compiler.release> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only scan for Java version pins in common build/CI/container files.
fd -HI -t f '(pom\.xml|Dockerfile|.*\.ya?ml|\.tool-versions|\.java-version)$' \
| xargs -r rg -n --no-heading 'maven.compiler.release|java-version|setup-java|temurin|openjdk|jdk|FROM .*java|FROM .*jdk'Repository: Maps-Messaging/canbus_interface
Length of output: 227
🏁 Script executed:
# Look for CI configuration files
find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name ".jdk-version" -o -name ".java-version" \) | head -20Repository: Maps-Messaging/canbus_interface
Length of output: 195
🏁 Script executed:
# Check for GitHub Actions workflows
fd -t f 'workflows' --type dRepository: Maps-Messaging/canbus_interface
Length of output: 57
🏁 Script executed:
# Look for Dockerfile or container files
fd -t f '(Dockerfile|docker-compose.*)'Repository: Maps-Messaging/canbus_interface
Length of output: 57
🏁 Script executed:
# Check for README or documentation
fd -t f '(README|CONTRIBUTING)' | head -10Repository: Maps-Messaging/canbus_interface
Length of output: 156
🏁 Script executed:
cat -n .buildkite/pipeline.ymlRepository: Maps-Messaging/canbus_interface
Length of output: 588
🏁 Script executed:
cat -n .buildkite/pipeline_release.ymlRepository: Maps-Messaging/canbus_interface
Length of output: 262
🏁 Script executed:
head -50 README.mdRepository: Maps-Messaging/canbus_interface
Length of output: 1678
🏁 Script executed:
grep -i "java\|jdk\|version" README.md | head -20Repository: Maps-Messaging/canbus_interface
Length of output: 304
Add explicit Java 21 version enforcement to Buildkite CI pipelines.
Line 83 sets maven.compiler.release to Java 21, but the Buildkite pipeline files (.buildkite/pipeline.yml and .buildkite/pipeline_release.yml) reference only a generic java_build_queue agent without specifying Java version. This creates a mismatch risk: if agents default to Java 11 or 17, the build compiles with target=21 on a lower JVM, potentially causing runtime failures. Add explicit Java 21 version enforcement in both pipeline files (e.g., via Buildkite agent environment setup or a versioned container image), and document the Java 21 requirement in README.md.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pom.xml` at line 83, pom.xml sets maven.compiler.release to 21 but Buildkite
pipelines don't enforce Java 21; update .buildkite/pipeline.yml and
.buildkite/pipeline_release.yml to ensure agents run Java 21 (for example select
a Java 21 container image, set an agent environment variable like JAVA_HOME
pointing to Java 21, or use a versioned build queue that guarantees Java 21) so
builds use the same JVM target as the maven.compiler.release setting, and add a
short note to README.md documenting the Java 21 requirement and any agent
selection instructions; reference maven.compiler.release in the README to
explain why Java 21 is required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
merge development to main for initial release
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation