Update IDL/License #18
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update restructures and modernizes the OPC Data Access (DA) interface definitions and their bindings. It removes the old Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IOPCServerList2
participant IOPCEnumGUID
Client->>IOPCServerList2: EnumClassesOfCategories(...)
IOPCServerList2-->>Client: IOPCEnumGUID
Client->>IOPCEnumGUID: Next/Skip/Reset/Clone
IOPCEnumGUID-->>Client: GUIDs / status
Client->>IOPCServerList2: GetClassDetails(clsid)
IOPCServerList2-->>Client: ProgID, UserType, VerIndProgID
Client->>IOPCServerList2: CLSIDFromProgID(ProgID)
IOPCServerList2-->>Client: CLSID
This diagram illustrates the enhanced OPC server enumeration and class detail retrieval using the new Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
opc_da_bindings/.metadata/generate.proj (2)
9-10: Keep naming convention consistent between WinMD output and inputsYou switched all inputs to lower‑case (
opcda.idl,opccomn.idl) but the produced WinMD is stillOPCDA.winmd(upper‑case).
While the file will be generated, the mixed‑case naming could cause confusion and grepping difficulties. Consider renaming toopcda.winmdfor symmetry, or add a comment explaining the deliberate deviation.
9-10: Missing explicit%(Identity)wildcard prevents incremental buildsHard‑coding two filenames inside the same
<Idls>and<Headers>item means MSBuild can’t track changes individually; a touch to one IDL forces regeneration of both headers/winmds.
Define them as two separateIdlitems – or use the built‑in%(Identity)metadata – to enable incremental recompilation.- <Idls Include="opcda.idl;opccomn.idl" /> + <Idls Include="opcda.idl" /> + <Idls Include="opccomn.idl" />opc_da_bindings/.metadata/OPCComn.idl (2)
124-131: Fix typos in documentation comments
impelmented→implemented; small polish but it appears in a public‑facing spec header.-// system configurations. IOPCEnumGUID is impelmented +// system configurations. IOPCEnumGUID is implemented
195-239: Consider exposing async interfaces or remove commented stubsCommented‑out async interfaces (
// interface AsyncIOPCServerList2;, etc.) clutter the type‑library section. Either:
- Provide real async UUIDs & definitions, or
- Remove the commented stubs to keep the IDL concise.
Leaving placeholders risks confusion in downstream bindings.
opc_da_bindings/.metadata/opcda.idl (1)
665-670: Spelling / naming consistency inOnWriteCompleteparametersThe parameter is spelled
hrMastererr, unlikehrMastererrorused in the two previous callbacks.
Although purely cosmetic, this breaks the otherwise consistent public interface and can trip code generators.-[in] HRESULT hrMastererr, +[in] HRESULT hrMastererror,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
opc_da_bindings/.metadata/OPCComn.idl(1 hunks)opc_da_bindings/.metadata/OPCDA_3.00.idl(0 hunks)opc_da_bindings/.metadata/generate.proj(1 hunks)opc_da_bindings/.metadata/main.cpp(1 hunks)opc_da_bindings/.metadata/opcda.idl(1 hunks)opc_da_bindings/src/bindings.rs(8 hunks)
💤 Files with no reviewable changes (1)
- opc_da_bindings/.metadata/OPCDA_3.00.idl
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: analyzing
🔇 Additional comments (14)
opc_da_bindings/.metadata/main.cpp (1)
3-4: Confirm header‑case strategy for cross‑platform buildsChanging
OPCComn.h→opccomn.handOPCDA_3.00.h→opcda.his fine on Windows (case‑insensitive) but will break *nix cross‑compiles if the physical filenames are still mixed‑case.
Double‑check the actual filenames emitted by MIDL / winmd‑generator and make sure CI (or future users) compiling on case‑sensitive filesystems won’t hit aNo such fileerror.Suggest adding a Linux‑container build to the pipeline to catch this class of issue early.
opc_da_bindings/.metadata/OPCComn.idl (1)
142-147: Attributes onNextout‑array are inconsistent with count parameter
Nextuses[size_is(celt), length_is(*pceltFetched)].
The standard pattern for enumerators is[size_is(celt), length_is(celtFetched)](without pointer indirection) to avoid dereferencing a nullpceltFetched.
Confirm that the Win32 headers generated by midlrt match the intended signature.opc_da_bindings/src/bindings.rs (12)
1943-2085: Good addition of theIOPCEnumGUIDinterfaceThis adds a standard COM-style enumeration interface specifically for GUIDs with the expected methods:
Next,Skip,Reset, andClone. The implementation follows the proper Rust COM binding patterns with appropriate vtable structures and trait definitions.
4326-4516: Well-structured implementation ofIOPCServerList2The new
IOPCServerList2interface properly extends the server list capabilities with methods for enumerating server classes, getting class details (with the added version-independent ProgID parameter), and converting ProgIDs to CLSIDs. The implementation correctly uses the newIOPCEnumGUIDinterface rather than the genericIEnumGUID.
5009-5011: Added appropriate consistency window state constantsThese string constants for consistency window states align with the updated OPC DA specification.
5017-5019: Swapped enum scope values to match updated specificationThe values for
OPC_ENUM_PRIVATE(changed from 5 to 4) andOPC_ENUM_PUBLIC(changed from 4 to 5) have been swapped to match the updated enumeration scope definitions in the new IDL.
5038-5038: Added property ID and description for consistency windowThis property addition aligns with the updated OPC DA specification.
Also applies to: 5057-5058
5040-5040: Added property ID and description for data filter valueThe data filter value property is properly defined with appropriate ID and description.
Also applies to: 5061-5062
5106-5107: Added dictionary propertiesDictionary and Dictionary ID properties are now properly included in the bindings.
Also applies to: 5068-5069
5127-5129: Added type system property definitionsType Description, Type ID, and Type System ID properties have been properly added with appropriate constants and descriptions.
Also applies to: 5093-5097
5130-5131: Added item identifier propertiesUnconverted Item ID and Unfiltered Item ID properties have been properly added with appropriate constants and descriptions.
Also applies to: 5098-5101
5133-5133: Added write behavior property definitionThe Write Behavior property is now properly defined with appropriate ID and description.
Also applies to: 5103-5104
5159-5160: Added type system string constantsOPCBinary and XMLSchema type system string constants have been added to support the updated specification.
5162-5164: Added write behavior descriptor constants"All or Nothing" and "Best Effort" write behavior string constants properly complement the write behavior property.
| interface IOPCCommon; | ||
| interface IOPCShutdown; | ||
| interface IOpcServerList; | ||
| interface IOPCServerList2; |
There was a problem hiding this comment.
Interface name case‑mismatch will break MIDL builds
In the type‑library section the declaration uses IOpcServerList, but the interface defined above is IOPCServerList (upper‑case “PC”). MIDL treats identifiers as case‑sensitive inside IDL.
- interface IOpcServerList;
+ interface IOPCServerList;📝 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.
| interface IOPCCommon; | |
| interface IOPCShutdown; | |
| interface IOpcServerList; | |
| interface IOPCServerList2; | |
| interface IOPCCommon; | |
| interface IOPCShutdown; | |
| interface IOPCServerList; | |
| interface IOPCServerList2; |
| typedef [unique] IOPCEnumGUID * LPOPCENUMGUID; | ||
|
|
||
| HRESULT Next( | ||
| [in] ULONG celt, | ||
| [out, size_is(celt), length_is(*pceltFetched)] GUID * rgelt, | ||
| [out] ULONG * pceltFetched | ||
| ); | ||
|
|
||
| HRESULT Skip( | ||
| [in] ULONG celt | ||
| ); | ||
|
|
||
| HRESULT Reset(); | ||
|
|
||
| HRESULT Clone( | ||
| [out] IOPCEnumGUID **ppenum | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Place typedefs outside the interface definition
typedef [unique] IOPCEnumGUID * LPOPCENUMGUID; is nested inside interface IOPCEnumGUID.
Although MIDL allows it, several compilers and language bindings (Rust/Bindgen, midlrt) ignore nested typedefs, leading to the pointer alias being unavailable.
Move it above the interface block:
-[
- object,
- ...
-]
-typedef [unique] IOPCEnumGUID * LPOPCENUMGUID;
-interface IOPCEnumGUID : IUnknown
+typedef [unique] IOPCEnumGUID * LPOPCENUMGUID;
+[
+ object,
+ ...
+]
+interface IOPCEnumGUID : IUnknown📝 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.
| typedef [unique] IOPCEnumGUID * LPOPCENUMGUID; | |
| HRESULT Next( | |
| [in] ULONG celt, | |
| [out, size_is(celt), length_is(*pceltFetched)] GUID * rgelt, | |
| [out] ULONG * pceltFetched | |
| ); | |
| HRESULT Skip( | |
| [in] ULONG celt | |
| ); | |
| HRESULT Reset(); | |
| HRESULT Clone( | |
| [out] IOPCEnumGUID **ppenum | |
| ); | |
| // Move this typedef above the interface declaration: | |
| typedef [unique] IOPCEnumGUID * LPOPCENUMGUID; | |
| [ | |
| object, | |
| ... | |
| ] | |
| interface IOPCEnumGUID : IUnknown | |
| { | |
| HRESULT Next( | |
| [in] ULONG celt, | |
| [out, size_is(celt), length_is(*pceltFetched)] GUID * rgelt, | |
| [out] ULONG * pceltFetched | |
| ); | |
| HRESULT Skip( | |
| [in] ULONG celt | |
| ); | |
| HRESULT Reset(); | |
| HRESULT Clone( | |
| [out] IOPCEnumGUID **ppenum | |
| ); | |
| }; |
| [ | ||
| uuid(8E368666-D72E-4f78-87ED-647611C61C9F), | ||
| pointer_default(unique) | ||
| ] |
There was a problem hiding this comment.
Add the [object] attribute so the interface is recognised as COM
Every other interface is declared with [object, uuid, …].
Without [object], IOPCGroupStateMgt2 is generated as a plain C struct and the v‑table isn’t emitted, preventing clients from QueryInterface‑ing for it.
-[
- uuid(8E368666-D72E-4f78-87ED-647611C61C9F),
- pointer_default(unique)
-]
+[
+ object,
+ uuid(8E368666-D72E-4f78-87ED-647611C61C9F),
+ pointer_default(unique)
+]| [in, string] LPWSTR szItemID, | ||
| [in,out, string] LPWSTR* pszContinuationPoint, | ||
| [in] DWORD dwMaxElementsReturned, |
There was a problem hiding this comment.
[string] cannot be applied to a double‑indirection parameter
pszContinuationPoint is an LPWSTR*. In MIDL the [string] attribute is valid for a single NUL‑terminated character array (e.g. LPWSTR), not for a pointer‑to‑pointer.
Keeping the attribute here produces midl : error MIDL2025 : syntax error : expecting a type specification.
-[in,out, string] LPWSTR* pszContinuationPoint,
+[in,out] LPWSTR* pszContinuationPoint,📝 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.
| [in, string] LPWSTR szItemID, | |
| [in,out, string] LPWSTR* pszContinuationPoint, | |
| [in] DWORD dwMaxElementsReturned, | |
| [in, string] LPWSTR szItemID, | |
| [in,out] LPWSTR* pszContinuationPoint, | |
| [in] DWORD dwMaxElementsReturned, |
| typedef enum tagOPCBROWSEFILTER | ||
| { | ||
| OPC_BROWSE_FILTER_ALL = 1, | ||
| OPC_BROWSE_FILTER_BRANCHES, | ||
| OPC_BROWSE_FILTER_ITEMS, | ||
| } | ||
| OPCBROWSEFILTER; |
There was a problem hiding this comment.
Remove the trailing comma to avoid a MIDL syntax error
OPCBROWSEFILTER is the only enumeration that ends with a trailing comma (OPC_BROWSE_FILTER_ITEMS,).
While C compilers accept this, midl.exe flags it as a syntax error.
typedef enum tagOPCBROWSEFILTER
{
OPC_BROWSE_FILTER_ALL = 1,
OPC_BROWSE_FILTER_BRANCHES,
- OPC_BROWSE_FILTER_ITEMS,
+ OPC_BROWSE_FILTER_ITEMS
}
OPCBROWSEFILTER;📝 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.
| typedef enum tagOPCBROWSEFILTER | |
| { | |
| OPC_BROWSE_FILTER_ALL = 1, | |
| OPC_BROWSE_FILTER_BRANCHES, | |
| OPC_BROWSE_FILTER_ITEMS, | |
| } | |
| OPCBROWSEFILTER; | |
| typedef enum tagOPCBROWSEFILTER | |
| { | |
| OPC_BROWSE_FILTER_ALL = 1, | |
| OPC_BROWSE_FILTER_BRANCHES, | |
| OPC_BROWSE_FILTER_ITEMS | |
| } | |
| OPCBROWSEFILTER; |
Summary by CodeRabbit
New Features
Refactor
Chores