-
Notifications
You must be signed in to change notification settings - Fork 3
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
[TIOVX-1501] Implementing node/graph level debug options #13
base: main
Are you sure you want to change the base?
Conversation
746dfea
to
bb472db
Compare
@@ -945,20 +947,23 @@ vx_status ownGraphScheduleGraphWrapper(vx_graph graph) | |||
VX_API_ENTRY vx_status VX_API_CALL vxScheduleGraph(vx_graph graph) | |||
{ | |||
vx_status status = (vx_status)VX_SUCCESS; | |||
vx_uint32 graph_debug_zonemask; | |||
|
|||
graph_debug_zonemask = graph->debug_zonemask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not to hide graph->debug_zonemask into VX_PRINT_LOCAL() macro?
Then the direct reference can be used like
VX_PRINT_LOCAL(VX_ZONE_ERROR, graph, "graph is already streaming\n");
instead of the pair
graph_debug_zonemask = graph->debug_zonemask;
VX_PRINT_LOCAL(VX_ZONE_ERROR, graph_debug_zonemask, "graph is already streaming\n");
The result will be little bit more compact
The same can be done for nodes, kernels etc. having the same field debug_zonemask
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, working on implementation now
|
||
#define VX_PRINT_LOCAL(zone, debug_zonemask, message, ...) do { tivx_print_object(((vx_enum)zone), debug_zonemask, "[%s:%u] " message, __FUNCTION__, __LINE__, ## __VA_ARGS__); } while (1 == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: change to
#define VX_PRINT_LOCAL(zone, object, message, ...) do { tivx_print_object(((vx_enum)zone), object->debug_zonemask, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with proposal, adding to implementation
* | ||
* \ingroup group_vx_graph | ||
*/ | ||
vx_status tivxGraphSetDebugZone(vx_graph graph, vx_uint32 debug_zone, vx_bool enable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I would like to consider the integration with our runtime debug configuration infrastructure. We use DLT Viewer (Diagnosis and Logging Tool Viewer, https://github.com/COVESA/dlt-viewer)
The tivxGraphSetDebugZone() and tivxNodeSetDebugZone() look acceptable to use with the DLT Viewer.
For the runtime configuration we need the list of the created OVX objects like graphs and nodes.
We can select the object of interest by its name and specify required debug mask using DLT callback feature.
DLT Viewer is usually started after the OVX executable is running. Problem: how can we control debug zone at runtime to debug Graph verify?
Another integration level would be to have DLT Context feature
One Nice to Have could be to make it possible to modify DLT viewer contect tree on Graph/Node creation registering application callback on the OVX object creation. Is it possible with the current TI OVX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new APIs can be called at any time, including after the graph verify phase. Therefore, changes can be made to the levels of specific nodes and graphs even at runtime. As far as DLT viewer integration goes with graph/node callbacks, we have filed tasks to explore DLT in greater depth in the next release. After doing so, we will be able to better address this point and work with you to understand this issue.
|
||
if(ownIsValidSpecificReference(vxCastRefFromGraph(graph), (vx_enum)VX_TYPE_GRAPH) != (vx_bool)vx_false_e) | ||
{ | ||
if (graph->is_streaming_enabled != 0) | ||
{ | ||
VX_PRINT(VX_ZONE_ERROR, "graph is already streaming\n"); | ||
VX_PRINT_LOCAL(VX_ZONE_ERROR, graph_debug_zonemask, "graph is already streaming\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this error message and the similar for graphs, nodes etc. there is no information which particular OVX object is affected. As OVX objects are named can we include object name into the debug output?
Ideally the object name output can be made a part of the VX_PRINT_LOCAL() macro.
Then the uniform debug output format from the macro can look like this:
``` [<Graph or Node Name>] "<Debug message>"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a good idea, working on implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After exploring further, this feature currently cannot be added. In order to include a node's name in the target kernel instance, a significant change to the framework's architecture would be required via an additional object descriptor structure within node object descriptors to carry the name of the host-side node they are spawned from across the remote cores. This would result in a substantial performance impact that we cannot introduce at this time. However, the prints will still include the function and line number from which they are called to provide additional information.
Ex:
VX_ZONE_WARNING: [ownAddTargetKernelInternal:449] May need to increase the value of TIVX_TARGET_KERNEL_MAX in tiovx/include/TI/tivx_config.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small update: I've discussed this topic with Lucas in greater detail, and we are planning to look into it further in the next release. We are all aligned with the value in including object names in their prints, so we will get back to you in the future as we progress on this feature.
No description provided.