Skip to content
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.

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

Support message headers #10

Merged
merged 24 commits into from
Nov 5, 2023
Merged

Support message headers #10

merged 24 commits into from
Nov 5, 2023

Conversation

kwitekrac
Copy link

@kwitekrac kwitekrac commented Oct 22, 2023

Description

This PR introduces support for using message headers in the publish and consume functions.

Important notes

  • The header keys and values are exposed as strings in the functions that LabVIEW will call. However, it's essential to note that in the C library, headers have specific value types, and currently, only "AMQP_FIELD_KIND_BYTES" (unformatted byte strings) is supported.
  • Transmitting and receiving an array of strings between LabVIEW and a shared library can present challenges. Therefore, our approach involves concatenating the strings within the array into a single string before transmission.
  • Additionally, the "queue create" function no longer automatically binds the queue; it now has its own separate method called "bind queue."

Related Issues

Checklist

  • I have used a PR title that is descriptive enough for a release note.
  • I have tested these changes locally.
  • I have added appropriate tests or updated existing tests.
  • I have tested these changes on a dedicated VM or a customer VM [name of the VM]
  • I have added appropriate documentation or updated existing documentation.

@kwitekrac kwitekrac added the feature This issue/PR relates to a feature request. label Oct 22, 2023
@chicco785 chicco785 requested a review from a team October 22, 2023 18:32
Copy link
Member

@chicco785 chicco785 left a comment

Choose a reason for hiding this comment

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

I have not knowledge on LabVIEW side to comment on integration aspects. As regards that I fully trust you :)

My only concern relates to the usage of separate arrays for keys and values, that if for any reason (e.g. no value) ends being of different length may result in some memory leak (and unwanted behaviour).

@AntoineChalons
Copy link

a few thoughts about headers :

  • adding support for all the possible types is a nice effort but not required at this point, it's if done, then good, if it still requires work, let's put it on hold
  • the only header that has to be read on the LabVIEW side for each timestamp (therefore at 50/60Hz) is the 'timestampId', so for this header it makes sense to put it as int64 (ou uint64), for the others there will be no impact on perf if they are passed as string and need to be type-cast on the LabVIEW side (it is the case for trigger header messages)

@kwitekrac kwitekrac marked this pull request as ready for review October 30, 2023 07:59
@kwitekrac
Copy link
Author

kwitekrac commented Oct 30, 2023

a few thoughts about headers :

  • adding support for all the possible types is a nice effort but not required at this point, it's if done, then good, if it still requires work, let's put it on hold
  • the only header that has to be read on the LabVIEW side for each timestamp (therefore at 50/60Hz) is the 'timestampId', so for this header it makes sense to put it as int64 (ou uint64), for the others there will be no impact on perf if they are passed as string and need to be type-cast on the LabVIEW side (it is the case for trigger header messages)

If you think about receiving the data, you also need to consider the type header that is sent as I8. Kevin will start working with Sending Messages from FL -> that will require to send UTF8. That is why I put a lot of effort into implementing most of the types that are defined in the library. At this point, only the following types are not supported:

  • AMQP_FIELD_KIND_BOOLEAN
  • AMQP_FIELD_KIND_DECIMAL
  • AMQP_FIELD_KIND_ARRAY
  • AMQP_FIELD_KIND_TABLE
  • AMQP_FIELD_KIND_VOID

as I cannot see the use case on our side.

.vscode/settings.json Outdated Show resolved Hide resolved
@kdevelleZ kdevelleZ self-requested a review October 31, 2023 11:12
labview/msg_headers.c Outdated Show resolved Hide resolved
@kwitekrac kwitekrac merged commit 09ba3dc into main Nov 5, 2023
2 checks passed
@kwitekrac kwitekrac deleted the support-message-headers branch November 5, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants