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

Add server to client remote log #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdavidsaver
Copy link
Member

Adds an interface for a server to send CMD_MESSAGE remote logging to the associated client.

Change client-side handling to print remote messages through pvxs/log.h API via the pvxs.remote.log logger. By default only mtype>=1 will be printed. This can be change by eg. PVXS_LOG=pvxs.remote.log=INFO.

Initially, add logging related to processing of pvRequest blobs. Client monitor pvRequest processing also gets a moderate reorganization.

@mdavidsaver
Copy link
Member Author

@kasemir fyi. I do not think you would not need to change anything in core.pva.

kasemir pushed a commit to ControlSystemStudio/phoebus that referenced this pull request Feb 3, 2025
Client always logged received CMD_MESSAGE, but lacked example for
issuing message from server

epics-base/pvxs#99
@kasemir
Copy link

kasemir commented Feb 3, 2025

.. change anything in core.pva.

Java client always logged received CMD_MESSAGE, but server lacked example for issuing message, so added that.

From https://docs.epics-controls.org/en/latest/pv-access/Protocol-Messages.html#cmd-message-0x12 it's not clear to me what the request ID represents. All other messages, if they have a request ID, it's from the client. The server then replies to some request with the client's request ID.
Is the CMD_MESSAGE request ID a server ID, simply counting up? Or is it also meant to be a client request ID, and the message then adds information to that client request?

@mdavidsaver
Copy link
Member Author

it's not clear to me what the request ID represents.

The usage in pvAccessCPP treats this as a IOID, aka. operation ID. So get/put/monitor/rpc.

https://github.com/epics-base/pvAccessCPP/blob/dafb6aad31a99f8d825a1f77aa919c82bb28e0cf/src/remoteClient/clientContextImpl.cpp#L2846-L2852

The documentation should certainly be clarified...

@mdavidsaver
Copy link
Member Author

The usage in pvAccessCPP treats this as a IOID, aka. operation ID. So get/put/monitor/rpc.

Which also implies a common requestID/IOID space for all operation types.

And also implies that CMD_MESSAGE can not be associated with channel independently of an operation.

I had been thinking to "cheat" in this later case and send requestID=0xffffffff while prepending the channel name to the message. However, pvAccessCPP prints "Orphaned server message ..." in this situation, I think which would only cause confusion.

@kasemir
Copy link

kasemir commented Feb 4, 2025

And also implies that CMD_MESSAGE can not be associated with channel independently of an operation.

Right, it can only reply to a specific request.
CMD_GET, CMD_PUT, .. already support replying with a Status, https://docs.epics-controls.org/en/latest/pv-access/Protocol-Encoding.html#status
The status is basically a message with type and message text, plus a call tree.
So for errors, a status reply might be more appropriate.
Even an OK status includes a message text where one could hint at unsupported request details.

Usage of CMD_MESSAGE seems limited to messages that only come up later, after the immediate reply to a request reported OK but then something else happened.

@mdavidsaver
Copy link
Member Author

So for errors, a status reply might be more appropriate.

Agreed. The situations this PR add logRemote() should all be warnings, where I think it is reasonable to give notice and continue.

And I take your point that the CMD_CREATE_CHANNEL reply Status could be used to send channel level warnings at creation time at least.

I will figure out how to change this PR to only add logRemote() to objects with an associated IOID.

@mdavidsaver
Copy link
Member Author

I will figure out how to change this PR to only add logRemote() to objects with an associated IOID.

This has been done, and I am happy with the result. However, merging now would introduce an ABI change. Since this is not an essential change, I'm going to leave it until after 1.3.3. (So the next next release will be 1.4.0)

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.

2 participants