Skip to content

Add ITestPlugin COM-RPC interface for thunder's new test support lib#493

Closed
smanes0213 wants to merge 5 commits intomasterfrom
development/test-support
Closed

Add ITestPlugin COM-RPC interface for thunder's new test support lib#493
smanes0213 wants to merge 5 commits intomasterfrom
development/test-support

Conversation

@smanes0213
Copy link
Copy Markdown

  • Add ITestPlugin.h with Echo/Greet methods and @JSON annotation
  • Register ID_TESTPLUGIN in QAIds.h at offset 0x060

Copilot AI review requested due to automatic review settings April 20, 2026 08:33
@smanes0213 smanes0213 marked this pull request as draft April 20, 2026 08:34
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new QualityAssurance COM-RPC/JSON-RPC interface (ITestPlugin) and assigns it a new QA interface ID, plus extends the example interface set with optional pin-scoped registration and a custom-error-code test interface.

Changes:

  • Added qa_interfaces/ITestPlugin.h (Echo/Greet) and registered ID_TESTPLUGIN in qa_interfaces/QAIds.h.
  • Updated example_interfaces/ISimpleInstanceObjects.h notification registration to accept an optional pin selector.
  • Added example_interfaces/ICustomErrorCode.h and registered ID_CUSTOMERRORCODE in example_interfaces/ExampleIds.h (also included in definitions/Definitions.cpp).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
qa_interfaces/QAIds.h Adds ID_TESTPLUGIN identifier for the new QA interface.
qa_interfaces/ITestPlugin.h Introduces the new QA ITestPlugin COM-RPC interface (Echo/Greet).
example_interfaces/ISimpleInstanceObjects.h Changes Register/Unregister API to accept optional pin scoping; adjusts notification signature annotations.
example_interfaces/ICustomErrorCode.h Adds an example interface for exercising custom vs non-custom error codes.
example_interfaces/ExampleIds.h Registers ID_CUSTOMERRORCODE for the new example interface.
definitions/Definitions.cpp Includes the new example interface header for definitions/registration builds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +63 to 65
virtual Core::hresult Register(INotification* const notification, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;
virtual Core::hresult Unregister(const INotification* const notificaiton, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;

Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDevice::Register / Unregister have been changed to include an additional pin argument. This is a breaking change for any existing COM-RPC/JSON-RPC clients generated against the previous interface signature. If backwards compatibility is required, consider introducing a new interface ID/version (or new methods) instead of modifying the existing method prototypes.

Suggested change
virtual Core::hresult Register(INotification* const notification, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;
virtual Core::hresult Unregister(const INotification* const notificaiton, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;
// Backward-compatible registration methods.
virtual Core::hresult Register(INotification* const notification) = 0;
virtual Core::hresult Unregister(const INotification* const notificaiton) = 0;
// Pin-aware registration methods added without changing the original RPC contract.
virtual Core::hresult RegisterPin(INotification* const notification, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;
virtual Core::hresult UnregisterPin(const INotification* const notificaiton, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;

Copilot uses AI. Check for mistakes.

enum { ID = ID_CUSTOMERRORCODE};

virtual Core::hresult TriggerCustomError(const int32_t errorcode) const = 0; // note: uin24_t would have been a better match, but as we also want to test overflow errors int32_t is used (otherwise generated code will already block the call)
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the inline note: uin24_t should be uint24_t (or another correct type name). Leaving the typo makes the comment confusing, especially since the note is explaining type choice.

Suggested change
virtual Core::hresult TriggerCustomError(const int32_t errorcode) const = 0; // note: uin24_t would have been a better match, but as we also want to test overflow errors int32_t is used (otherwise generated code will already block the call)
virtual Core::hresult TriggerCustomError(const int32_t errorcode) const = 0; // note: uint24_t would have been a better match, but as we also want to test overflow errors int32_t is used (otherwise generated code will already block the call)

Copilot uses AI. Check for mistakes.
Comment thread qa_interfaces/QAIds.h
ID_TESTCUSTOM = ID_TESTTEXTOPTIONS + 6,
ID_TESTCUSTOM_NOTIFICATION = ID_TESTTEXTOPTIONS + 7
ID_TESTCUSTOM_NOTIFICATION = ID_TESTTEXTOPTIONS + 7,

Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ID_TESTPLUGIN is assigned at ... + 0x060, skipping the default 0x010 group increment pattern used earlier in this file (0x000/0x010/0x020/0x030). If the gap is intentional (e.g., reserving 0x040/0x050 for future groups), add a short comment explaining why; otherwise consider using ... + 0x040 to follow the documented convention in the header.

Suggested change
// Reserve 0x040 and 0x050 for future QA interface groups.

Copilot uses AI. Check for mistakes.
Comment on lines 100 to +103
#include <example_interfaces/ISimpleInstanceObjects.h>
#include <example_interfaces/ISimpleAsync.h>
#include <example_interfaces/ISimpleCustomObjects.h>
#include <example_interfaces/ICustomErrorCode.h>
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description only mentions adding the QA ITestPlugin interface/ID, but this change set also modifies example interfaces (e.g., new ICustomErrorCode, ExampleIds updates, and ISimpleInstanceObjects signature changes). Please update the PR description to reflect these additional changes, or split them into separate PRs to keep review/rollback scope clear.

Copilot uses AI. Check for mistakes.
virtual Core::hresult Register(INotification* const notification) = 0;
virtual Core::hresult Unregister(const INotification* const notificaiton) = 0;
virtual Core::hresult Register(INotification* const notification, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;
virtual Core::hresult Unregister(const INotification* const notificaiton, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Unregister parameter name is misspelled as notificaiton. This typo will propagate into generated JSON-RPC / proxy-stub symbols and makes the API harder to use. Rename it to notification for consistency with Register and other interfaces.

Suggested change
virtual Core::hresult Unregister(const INotification* const notificaiton, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;
virtual Core::hresult Unregister(const INotification* const notification, const Core::OptionalType<uint8_t>& pin /* @index */) = 0;

Copilot uses AI. Check for mistakes.
@smanes0213 smanes0213 closed this Apr 20, 2026
@smanes0213 smanes0213 deleted the development/test-support branch April 20, 2026 11:38
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants