Add command builders for the DI object#31
Conversation
'Command builders' for the Diagnostic Object has been added. I.e. functions that writes a correctly formatted command to a given message buffer. The host application must still send the command, that is not handled by those builder functions.
There was a problem hiding this comment.
Pull request overview
This PR adds command builder functions for the Diagnostic Object (DI) in the Anybus CompactCom driver API. These builder functions format diagnostic event commands for the ABCC, including support for generic diagnostic events, extended diagnostic data, and PROFINET-specific channel diagnostics. The host application remains responsible for sending the commands after they are built.
Changes:
- Added diagnostic_object.c/.h files with builder functions for creating diagnostic event commands
- Integrated the new files into the build systems (cmake and mk)
- Included validation logic for PROFINET-specific diagnostic parameters
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/anybus_objects/diagnostic_object.c | Implementation of DI command builders with validation functions for PROFINET parameters |
| inc/anybus_objects/diagnostic_object.h | Public API declarations, constants, and extensive usage examples for DI command builders |
| abcc-driver-api.mk | Added diagnostic_object.c to the makefile build sources |
| abcc-driver-api.cmake | Added diagnostic_object.c/.h to the CMake build system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* | ||
| ** DI_BuildCmdAddExtDiag() is supposed to be called after | ||
| ** DI_BuildCmdCreateDiag() as written its data to the message buffer. |
There was a problem hiding this comment.
The comment contains a grammatical error: "as written" should be "has written" or "after it has written".
| ** DI_BuildCmdCreateDiag() as written its data to the message buffer. | |
| ** DI_BuildCmdCreateDiag() has written its data to the message buffer. |
| { | ||
| /* | ||
| ** The caller can not select the ChannelProperties.Specifier value. | ||
| ** It must be locked to Appears, and the ABCC generated the Disappears |
There was a problem hiding this comment.
The comment contains a grammatical error: "the ABCC generated the Disappears" should be "the ABCC generates the Disappears".
| ** It must be locked to Appears, and the ABCC generated the Disappears | |
| ** It must be locked to Appears, and the ABCC generates the Disappears |
| { 2, PD_READ, AD_MAP_ALL_ELEM, 0 }, | ||
|
|
||
| /* | ||
| ** Now, our monitoring hardware for the outputs has detected a short circut on |
There was a problem hiding this comment.
The comment contains a typo: "short circut" should be "short circuit".
| ** Now, our monitoring hardware for the outputs has detected a short circut on | |
| ** Now, our monitoring hardware for the outputs has detected a short circuit on |
| ** | ||
| ** NOTE: | ||
| ** The functions, constants, bitmasks, values, and descriptions here are not | ||
| ** intended to be a substitue for the PROFINET specification. It is assumed |
There was a problem hiding this comment.
The comment contains a typo: "substitue" should be "substitute".
| ** intended to be a substitue for the PROFINET specification. It is assumed | |
| ** intended to be a substitute for the PROFINET specification. It is assumed |
| iTemp = ( iTemp & ~( DI_PRT_CHAN_PROP_MAINT_MASK ) ) | DI_PRT_CHAN_PROP_MAINT_QUALIFIED; | ||
| ABCC_SetMsgData16( psMsg, iTOiBe( iTemp ), ( iOffset - ( DI_PRT_CHAN_DIAG_LEN + DI_PRT_EXT_CHAN_DIAG_LEN ) ) + ABP_UINT16_SIZEOF + ABP_UINT16_SIZEOF ); | ||
|
|
||
| lTemp = 1 << bChannelQualifier; |
There was a problem hiding this comment.
The expression 1 << bChannelQualifier is potentially problematic. When bChannelQualifier is 31, shifting the integer literal 1 (which is of type int) left by 31 bits results in undefined behavior in C if int is 32 bits (the sign bit would be set). The literal should be explicitly unsigned or of type UINT32. Use 1U << bChannelQualifier or (UINT32)1 << bChannelQualifier instead.
| lTemp = 1 << bChannelQualifier; | |
| lTemp = ( (UINT32)1 << bChannelQualifier ); |
| #include "abcc.h" | ||
|
|
There was a problem hiding this comment.
The macro DI_OBJ_ENABLE is used but never defined anywhere in the codebase. This will cause compilation issues or incorrect behavior. Following the pattern of other objects in the codebase (e.g., ANB_FSI_OBJ_ENABLE), the header file should include "../../src/abcc_api_config.h" before checking DI_OBJ_ENABLE, and DI_OBJ_ENABLE should be defined in src/abcc_api_config.h with a default value (typically 0).
| #include "abcc.h" | |
| #include "abcc.h" | |
| #include "../../src/abcc_api_config.h" | |
| #ifndef DI_OBJ_ENABLE | |
| #define DI_OBJ_ENABLE 0 | |
| #endif |
| return( ABCC_EC_NO_ERROR ); | ||
| } | ||
|
|
||
| ABCC_ErrorCodeType DI_BuildCmdAddExtDiag( ABP_MsgType* psMsg, UINT16 iSlot, UINT16 iAdi, UINT8 bElement, UINT8 bBitInElement ) |
There was a problem hiding this comment.
The parameter name in the function declaration (bBit) doesn't match the implementation (bBitInElement). While this is technically allowed in C, it creates inconsistency and potential confusion. The parameter names should match between the header and source file for better maintainability.
| ABCC_ErrorCodeType DI_BuildCmdAddExtDiag( ABP_MsgType* psMsg, UINT16 iSlot, UINT16 iAdi, UINT8 bElement, UINT8 bBitInElement ) | |
| ABCC_ErrorCodeType DI_BuildCmdAddExtDiag( ABP_MsgType* psMsg, UINT16 iSlot, UINT16 iAdi, UINT8 bElement, UINT8 bBit ) |
|
|
||
| /*------------------------------------------------------------------------------ | ||
| ** Perform some basic sanity checks on the PROFINET-specific values that goes | ||
| ** int a ChannelDiagnosis of ExtChannelDiagnosis structure. |
There was a problem hiding this comment.
The comment contains a typo: "int a ChannelDiagnosis" should be "into a ChannelDiagnosis".
| ** int a ChannelDiagnosis of ExtChannelDiagnosis structure. | |
| ** into a ChannelDiagnosis of ExtChannelDiagnosis structure. |
| return( FALSE ); | ||
| } | ||
|
|
||
| if( ( ( iChannelProperties & ~(DI_PRT_CHAN_PROP_TYPE_MASK) ) != DI_PRT_CHAN_PROP_TYPE_UNDEF ) && |
There was a problem hiding this comment.
The bitwise logic appears incorrect. The condition checks if (iChannelProperties & ~(DI_PRT_CHAN_PROP_TYPE_MASK)) != DI_PRT_CHAN_PROP_TYPE_UNDEF. However, DI_PRT_CHAN_PROP_TYPE_UNDEF is 0x0000, and after masking out TYPE_MASK (0x00FF), any non-zero value in the upper bits would pass this check. The intent seems to be checking if the Type field is NOT Undefined, but the logic should be: ((iChannelProperties & DI_PRT_CHAN_PROP_TYPE_MASK) != DI_PRT_CHAN_PROP_TYPE_UNDEF).
| if( ( ( iChannelProperties & ~(DI_PRT_CHAN_PROP_TYPE_MASK) ) != DI_PRT_CHAN_PROP_TYPE_UNDEF ) && | |
| if( ( ( iChannelProperties & DI_PRT_CHAN_PROP_TYPE_MASK ) != DI_PRT_CHAN_PROP_TYPE_UNDEF ) && |
| */ | ||
|
|
||
| /* | ||
| ** Build a 'Create diag. event' command with a "major, recovrable" severity, |
There was a problem hiding this comment.
The comment contains a typo: "recovrable" should be "recoverable".
| ** Build a 'Create diag. event' command with a "major, recovrable" severity, | |
| ** Build a 'Create diag. event' command with a "major, recoverable" severity, |
'Command builders' for the Diagnostic Object has been added. I.e. functions that writes a correctly formatted command to a given message buffer. The host application must still send the command, that is not handled by those builder functions.