Skip to content

Opencmw-cpp interoperability fixes and testing #239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
wirew0rm merged 4 commits into from
Jan 13, 2026
Merged

Opencmw-cpp interoperability fixes and testing #239

wirew0rm merged 4 commits into from
Jan 13, 2026

Conversation

@wirew0rm
Copy link
Member

@wirew0rm wirew0rm commented Dec 19, 2025 •
edited
Loading

Follow-up of #223 and companion PR to fair-acc/opencmw-cpp#389
Should be merged after that one, to ensure that the compatibility test is correctly run.

Changes:

  • metadata: remove direction and groups metadata fields and add modifier field, to match opencmw-cpp
  • add quantity metadata field
  • serialise char as 8bit wide character as opposed to java, which serialises it as a 16bit wide character
  • fetch a simple test binary from the opencmw-cpp repo and use it to verify cross compatibility

missing functionality:

  • fetch test binary from opencmw-cpp release artifacts
  • fix serialiser test failure (probably an errror in the test implementation since the deserialisation of subscription updates works)
  • find out why tests fail/flake in the CI. (they run fine locally and do not really seem to be directly connected)

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 19, 2025
View reviewed changes
@wirew0rm wirew0rm temporarily deployed to configure coverage January 5, 2026 16:33 — with GitHub Actions Inactive
@wirew0rm wirew0rm had a problem deploying to configure coverage January 5, 2026 16:33 — with GitHub Actions Failure
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 5, 2026
View reviewed changes
@wirew0rm wirew0rm had a problem deploying to configure coverage January 5, 2026 16:50 — with GitHub Actions Failure
@wirew0rm wirew0rm had a problem deploying to configure coverage January 5, 2026 16:58 — with GitHub Actions Failure
@wirew0rm wirew0rm had a problem deploying to configure coverage January 5, 2026 16:58 — with GitHub Actions Failure
@wirew0rm wirew0rm had a problem deploying to configure coverage January 5, 2026 17:14 — with GitHub Actions Failure
@wirew0rm wirew0rm had a problem deploying to configure coverage January 5, 2026 17:14 — with GitHub Actions Failure
@wirew0rm wirew0rm temporarily deployed to configure coverage January 5, 2026 17:19 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to configure coverage January 5, 2026 17:19 — with GitHub Actions Inactive
@wirew0rm wirew0rm marked this pull request as ready for review January 6, 2026 08:17
RalphSteinhagen
RalphSteinhagen reviewed Jan 12, 2026
View reviewed changes
RalphSteinhagen
RalphSteinhagen approved these changes Jan 12, 2026
View reviewed changes
Copy link
Member

@RalphSteinhagen RalphSteinhagen left a comment

Choose a reason for hiding this comment

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

OK...

While from a design point of view, the type hierarchy could be stream-lined a bit more into signed/unsigned and 8, 16, 32, 64-bit width fundamental integer, + float + string types, we should leave the ambiguity of byte and char (Java vs. C++, i.e. Java 16 bit, vs. C/C++ 8 bit/UTF8, or wchar) since this is an unlikely but possible feature that FESA/Japc developer/user might rely upon.

Can be merged as is. Optional to be verified:

  • missing newline at the end of files (cosmetic nag by GH inspections)
  • missing @Override and other security bot complaints
  • JSON iter dependency (i.e. do we need to explicitly pull this in as a primary dependency. AFAIK this is only needed for testing/benchmarking).

@wirew0rm
BinarySerialiser: Update Metadata to sync with cpp
6fa0dbb 
Opencmw-java and opencmw-cpp used different metadata entries for the
field descriptions. While opencmw-cpp serialised [description(string),
unit(string), modifier(byte/enum)], opencmw-java used
[description(string), unit(string), direction(string),
groups(string[])].
This lead to failures in deserialising c++ serialised field medatadata.

These changes make opencmw-java use the same format as c++.

Signed-off-by: Alexander Krimm <A.Krimm@gsi.de>
@wirew0rm
BinarySerialiser: add quantity metadata field
fd7d2fa 
Signed-off-by: Alexander Krimm <A.Krimm@gsi.de>
@wirew0rm
Serialise char as single byte like c++
7a10655 
char in c++ is usually represented as 1 byte, while java chose 2 bytes.
To allow for interoperability, this changes the serialisation to only
use a single byte.

Since this replaces a memcpy by a naive for loop, it could probably be
improved by doing it in a tight loop, and using more efficient
conversion functions. This first step gets the 2 implementations to be
consistent, performance improvements can be added later on.

Signed-off-by: Alexander Krimm <A.Krimm@gsi.de>
@wirew0rm
Add a test to ensure interoperability with the c++ version
14847e6 
To run the tests locally, copy the Compatibility test executable from
opencmw-cpp to the repository root.
The CI will fetch it from the artifact of opencmw-cpp.
If it is not available, the tests will be skipped.

Signed-off-by: Alexander Krimm <A.Krimm@gsi.de>
@wirew0rm wirew0rm temporarily deployed to configure coverage January 13, 2026 08:29 — with GitHub Actions Inactive
@wirew0rm wirew0rm merged commit e5c13e9 into main Jan 13, 2026
9 of 15 checks passed
@wirew0rm wirew0rm deleted the interop-tests branch January 13, 2026 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalphSteinhagen RalphSteinhagen RalphSteinhagen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wirew0rm @RalphSteinhagen