Gedare Bloom started a new discussion: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/282#note_114006


I will try to take a pass through the code changes soon, but I wanted to give 
my first impression is that this is a big API-level change to be trying to get 
in before the release of 6.1. It's not clear to me though which of these 
changes are for external use, or for the internal implementation. 

The API changes I noticed:
- new function `rtems_record_fetch_recommended_item_count()`
- new function `rtems_record_fetch_initialize()`
- new function `rtems_record_fetch()`
- new structure `rtems_record_fetch_control`
- new type (enum) `rtems_record_fetch_status`
- structure name change `uptime` to `rtems_record_client_uptime` (this is an 
improvement to avoid polluting the global namespace)
- return type change for `rtems_record_client_init`
- increment the `RTEMS_RECORD_THE_VERSION` with associated restructuring of the 
`rtems_record_event` structure

And the deprecated and obsoleted call:
- `rtems_record_drain`
- `rtems_record_writev`

It is unusual to directly deprecate and obsolete an API call, but, it seems 
this function was not documented and was not being used by the external 
tooling, so I'm willing to let that slide. It should still be noted and treated 
as a deprecation, even if it was a "hidden interface" just on the chance 
someone happened to be using it for something.

Relatedly, it would help as part of this refactoring if you can keep the public 
API cleanly defined in installed `cpukit/include/record*.h` headers, and hide 
the implementation details a bit more. This API leaks a lot of implementation 
details and doesn't offer a great abstraction for creating the client-side 
tools, as seen in the associated 
https://gitlab.rtems.org/rtems/tools/rtems-tools/-/merge_requests/36/ which 
basically duplicates a lot of this MR. It's not clear to me why the header 
files are being duplicated between rtems.git and rtems-tools.git, instead of 
having the tool pick up the header from an installed build. This expectation 
should be documented within the header file.

I would like to see some better separation between what you view as the "stable 
API" of the `rtems_record_xxx` interface, and what may be a bit more flexible. 
It is good the tooling is updated also, but if someone created their own tools 
based on the Record Interface, those would become badly broken. That is 
unlikely but I'm taking a more careful view about API changes these days. 

I can't tell if these changes need updates to the documentation in 
`user/tracing/eventrecording.rst` also, as I cannot tell what parts of the 
header files are internal and what parts are for the external/tools use.

-- 
View it on GitLab: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/282#note_114006
You're receiving this email because of your account on gitlab.rtems.org.


_______________________________________________
bugs mailing list
[email protected]
http://lists.rtems.org/mailman/listinfo/bugs

Reply via email to