Skip to content

Make parameter API synchronous#80

Merged
madelen-at-work merged 1 commit intomainfrom
non-stream-param
Aug 27, 2024
Merged

Make parameter API synchronous#80
madelen-at-work merged 1 commit intomainfrom
non-stream-param

Conversation

@madelen-at-work
Copy link
Contributor

@madelen-at-work madelen-at-work commented Aug 15, 2024 •
edited
Loading

Describe your changes

To align with usage (i.e the Parameter API example, we change the implementation of the Parameter API to synchronous gRPC communication.

Checklist before requesting a review

  • I have performed a self-review of my own code
  • I have verified that the code builds perfectly fine on my local system
  • I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have verified that my code follows the style already available in the repository
  • I have made corresponding changes to the documentation

@madelen-at-work madelen-at-work requested review from a team as code owners August 15, 2024 10:06
@madelen-at-work madelen-at-work changed the base branch from main to parameter-api August 15, 2024 10:07
killenheladagen
killenheladagen reviewed Aug 16, 2024
View reviewed changes
src/parameter.cpp Outdated

char parhand_result[BUFSIZ];
// Initialize Parameter Service
bool Parameter::Init(const bool verbose)
Copy link
Contributor

@killenheladagen killenheladagen Aug 16, 2024

Choose a reason for hiding this comment

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

If Parameter::Init was instead the class constructor, throwing runtime_error on failure, there wouldn't be a need to check ax_parameter == NULL before assignment and it wouldn't have to be checked at destruction. But maybe it's not worth the effort.

killenheladagen
killenheladagen reviewed Aug 16, 2024
View reviewed changes
src/parameter.cpp Outdated
}
Status Parameter::GetValues(ServerContext *context, const Request *request, Response *response)
{
if (ax_parameter == NULL)
Copy link
Contributor

@killenheladagen killenheladagen Aug 16, 2024

Choose a reason for hiding this comment

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

Maybe it actually is worth the effort, because by initializing ax_parameter in the constructor, it is guaranteed to be always initialized.

Copy link
Contributor Author

@madelen-at-work madelen-at-work Aug 19, 2024

Choose a reason for hiding this comment

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

I'm not sure it is - this API has so far followed the same 'pattern' as the two other (i.e. using Init instead of a constructor). I don't see any real benefit in refactoring this API only.

killenheladagen
killenheladagen reviewed Aug 16, 2024
View reviewed changes
src/parameter.cpp Outdated
{
ERRORLOG << "Error when getting axparameter: " << _error->message << endl;
parameter_value = g_strdup("");
g_clear_error(&_error);
Copy link
Contributor

@killenheladagen killenheladagen Aug 16, 2024

Choose a reason for hiding this comment

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

Why doesn't this lead to returning an error code?

Copy link
Contributor Author

@madelen-at-work madelen-at-work Aug 19, 2024

Choose a reason for hiding this comment

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

This is inline with how the API works now - as long as the format of the key is correct it will return an empty value and gRPC Status::OK

killenheladagen
killenheladagen reviewed Aug 16, 2024
View reviewed changes
killenheladagen
killenheladagen reviewed Aug 16, 2024
View reviewed changes
test/parameter_test.cc Outdated
struct tm *nTime = localtime(&now);
int nowTime = nTime->tm_hour;
if (nowTime != logTime)
namespace parameter_test
Copy link
Contributor

@killenheladagen killenheladagen Aug 16, 2024

Choose a reason for hiding this comment

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

Starting with C++17, you can write this as namespace acap_runtime::parameter_test

killenheladagen
killenheladagen reviewed Aug 16, 2024
View reviewed changes
@madelen-at-work madelen-at-work force-pushed the parameter-api branch 2 times, most recently from 081c963 to 1fb55df Compare August 19, 2024 09:23
@madelen-at-work madelen-at-work force-pushed the parameter-api branch 2 times, most recently from 5ab3e6e to e69d6cd Compare August 20, 2024 10:46
Base automatically changed from parameter-api to main August 27, 2024 13:48
@madelen-at-work madelen-at-work merged commit 7babb34 into main Aug 27, 2024
@madelen-at-work madelen-at-work deleted the non-stream-param branch August 27, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@killenheladagen killenheladagen killenheladagen approved these changes

+1 more reviewer

@Corallo Corallo Corallo approved these changes

Reviewers whose approvals may not affect merge requirements

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

@madelen-at-work @killenheladagen @Corallo