Install fastrpc headers to installdir #163
Conversation
Few of the fastrpc header files are used by developers for their projects. Copy these headers from source directory to target directory to ensure that they are available for developers to include in their own projects. Signed-off-by: Ekansh Gupta <[email protected]>
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_pls.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_debug.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/dspqueue.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/adsp_default_listener1.h |
There was a problem hiding this comment.
Which of the headers actually define an API that is useable by the users of fastrpc?
There was a problem hiding this comment.
All these headers are maintained as shippable components and are utilized by various clients. Specifically, for QLI supporting platforms, some headers are not used. However, generally, all headers are provided for users of fastrpc.
There was a problem hiding this comment.
I quickly went through the headers. Some of them are dsprpcd-internal headers. Some are bad wrappers which should have been dropped long ago. So, no, please don't randomly export those headers.
There was a problem hiding this comment.
All these headers are already provided in dspservices-headers_15.0.qcom_aarch64.tar.gz prebuilt (https://docs.qualcomm.com/bundle/publicresource/topics/RNO-250403001134/ReleaseNote.html?vproduct=1601111740013072&version=1.4)
I'm checking the impact on clients if I restrict exporting any headers.
| otherinclude_HEADERS = $(top_srcdir)/inc/*.h | ||
| # Export fastrpc headers | ||
| fastrpc_includedir = $(includedir) | ||
| fastrpc_include_HEADERS = $(top_srcdir)/inc/AEEatomic.h |
There was a problem hiding this comment.
No. We don't need another atomic wrapper, users should use -latomic instead. Please don't export this one
| # Export fastrpc headers | ||
| fastrpc_includedir = $(includedir) | ||
| fastrpc_include_HEADERS = $(top_srcdir)/inc/AEEatomic.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEQList.h |
There was a problem hiding this comment.
Yet another list header. It is not a DSP interface, so please drop it from the exported headers. If necessary, people can use <sys/queue.h>
| fastrpc_includedir = $(includedir) | ||
| fastrpc_include_HEADERS = $(top_srcdir)/inc/AEEatomic.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEQList.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEstd.h |
There was a problem hiding this comment.
A random collection of libc wrappers and some extra functions. Drop it and use libc directly. Don't encourage our users to use unnecessary libc wrappers.
| fastrpc_include_HEADERS = $(top_srcdir)/inc/AEEatomic.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEQList.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEstd.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEStdDef.h |
There was a problem hiding this comment.
This is for those that don't know about stddef.h / stdint.h. Please drop it completely.
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEstd.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEStdDef.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEStdErr.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEVaList.h |
There was a problem hiding this comment.
Use stdarg.h instead, drop the header.
| fastrpc_include_HEADERS += $(top_srcdir)/inc/remote.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/remote64.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/rpcmem.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/verify.h |
There was a problem hiding this comment.
This is an internal header, don't export it to outside users.
There was a problem hiding this comment.
this header is used by sensors and audio daemons.
There was a problem hiding this comment.
Why? It's definitely an internal header, depending on the implementation details (like the use of liblog, FARF, syslog or just printf). If any app wants to use it, it should vendor in this header: it doesn't provide an API, it is not directly related to the fastrpc, etc. So, as the fastrpc wasn't installing any headers beforehand, it should be excluded.
| fastrpc_include_HEADERS += $(top_srcdir)/inc/remote64.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/rpcmem.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/verify.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_farf.h |
There was a problem hiding this comment.
This one also looks like an internal header, rather than something that should be used by library users.
There was a problem hiding this comment.
I see this is included by multiple clients like sensors, CV etc.
| fastrpc_include_HEADERS += $(top_srcdir)/inc/rpcmem.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/verify.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_farf.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_pls.h |
There was a problem hiding this comment.
- Warning, this API should only be called from within a thread started by FastRPC, and not from any user created threads via the qurt apis.
So, this is not the API for apps.
| fastrpc_include_HEADERS += $(top_srcdir)/inc/verify.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_farf.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_pls.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_debug.h |
There was a problem hiding this comment.
Is it a debug for adsprpcd or for user apps?
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_pls.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_debug.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/dspqueue.h | ||
| fastrpc_include_HEADERS += $(top_srcdir)/inc/adsp_default_listener1.h |
There was a problem hiding this comment.
adsp_default_listener_register is not documented at all. What is it?
There was a problem hiding this comment.
I'll add documentation for the header as a separate commit.
|
accidently closed this PR |
Few of the fastrpc header files are used by developers for their projects. Copy these headers from source directory to target directory to ensure that they are available for developers to include in their own projects.