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

adding support for sideband streaming #1113

Merged
merged 9 commits into from
Nov 6, 2024

Conversation

asumit
Copy link
Contributor

@asumit asumit commented Oct 24, 2024

What does this Pull Request accomplish?

This PR adds infrastructure to add streaming support for respective driver apis.

We are adding moniker service source files directly in this repo as a stop gap to enable submission of driver specific
streaming support. We would reference the source files directly from the sideband repo as a subsequent PR and remove thse from the source in the grpc-device repo.
The streaming functionality is aided by the grpc-sideband implementation that is added as a submodule to the grpc-device repo.
This PR build is subject to succesful completion of 2 PRs on the grpc-sideband repo

  1. disable warnings for conversion between int64_t to int and size_t to int grpc-sideband#7 : This PR disables warnings ( being treated as an error during build in the sideband repo due to coersion of data types
    int64_t -> int and size_t -> int
  2. https://github.com/ni/grpc-sideband/pull/6/files : This PR adds the option of statically building the ni_grpc_sideband lib.

grpc-device is expected to link statically against the sideband lib

The following changes are present in this PR.

  1. Option INCLUDE_SIDEBAND_RDMA has been set to OFF to disable RDMA functionality in the sideband lib.
  2. Option SIDEBAND_STATIC has been set to ON to enable building the sideband lib as a static lib.
  3. moniker service source files (data_moniker.proto, data_moniker_service.cpp and data_moniker_service.h) have been added directly in this repo
  4. The new moniker service runs on a separate thread on port 50055
  5. core_server.cpp introduces the sideband read write loop that is set up to perform the read write loop on the device.
  6. core_server.cpp also has RT specific code changes to add schedule affinity on different cores to get low latency on the read/ write steps.
  7. Also introduced additional configuration property in the server_config.json file to set the address of the sideband service. server side needs to set the host machines IP address in there.
  8. Moniker service is registered inside the core_services_registrar.cpp.
  9. Moniker service is enabled by default and not hidden under faeture toggle.
  10. sideband feature is added under a feature toggle. Please add either "feature_toggles": { "sidebandserver": true } or "code_readiness": "nextrelease" in the server_config.json file to enable this feature

Why should this Pull Request be merged?

What testing has been done?

Driver specific streaming apis will be added later. hence testing is only done to ensure that the moniker service is up and running and clients are able to call BeginSidebandStream without error.

Regular driver api for daq works fine as it is.

Copy link
Collaborator

@astarche astarche left a comment

Choose a reason for hiding this comment

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

I would like to look at this again after you have answered my questions and gotten feedback from at least 1 other reviewer.

source/server/server_configuration_parser.cpp Outdated Show resolved Hide resolved
source/server/server_configuration_parser.cpp Outdated Show resolved Hide resolved
source/protobuf/data_moniker.proto Outdated Show resolved Hide resolved
@asumit asumit requested a review from astarche October 29, 2024 22:43
source/server/core_server.cpp Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
source/protobuf/data_moniker.proto Outdated Show resolved Hide resolved
source/config/localhost_config.json Show resolved Hide resolved
source/server/core_server.cpp Outdated Show resolved Hide resolved
source/server/core_services_registrar.cpp Outdated Show resolved Hide resolved
source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved
source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved
source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved
source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved
@amehra-ni amehra-ni requested a review from astarche November 4, 2024 11:01
source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@amehra-ni amehra-ni requested a review from maxxboehme November 5, 2024 08:21
@amehra-ni amehra-ni merged commit bd1ba15 into main Nov 6, 2024
10 checks passed
@amehra-ni amehra-ni deleted the users/sagrahar/add-sideband-support branch November 6, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants