Skip to content

Install fastrpc headers to installdir #163

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.

Sign up for GitHub

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

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

Install fastrpc headers to installdir #163

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,5 +129,20 @@ sdsprpcd_CFLAGS = -I$(top_srcdir)/inc -DDEFAULT_DOMAIN_ID=2 -DUSE_SYSLOG -DNO_HA
sdsprpcd_LDADD = -ldl $(USE_LOG)


otherincludedir = $(includedir)/fastrpc
otherinclude_HEADERS = $(top_srcdir)/inc/*.h
# Export fastrpc headers
fastrpc_includedir = $(includedir)
fastrpc_include_HEADERS = $(top_srcdir)/inc/AEEatomic.h
Copy link
Contributor

Choose a reason for hiding this comment

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

No. We don't need another atomic wrapper, users should use -latomic instead. Please don't export this one

fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEQList.h
Copy link
Contributor

Choose a reason for hiding this comment

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

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_include_HEADERS += $(top_srcdir)/inc/AEEstd.h
Copy link
Contributor

Choose a reason for hiding this comment

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

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/AEEStdDef.h
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for those that don't know about stddef.h / stdint.h. Please drop it completely.

fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEStdErr.h
fastrpc_include_HEADERS += $(top_srcdir)/inc/AEEVaList.h
Copy link
Contributor

Choose a reason for hiding this comment

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

Use stdarg.h instead, drop the header.

fastrpc_include_HEADERS += $(top_srcdir)/inc/remote.h
fastrpc_include_HEADERS += $(top_srcdir)/inc/remote64.h
Copy link
Contributor

Choose a reason for hiding this comment

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

"remote64.h will be deleted in future". Please stop exporting it.

fastrpc_include_HEADERS += $(top_srcdir)/inc/rpcmem.h
fastrpc_include_HEADERS += $(top_srcdir)/inc/verify.h
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an internal header, don't export it to outside users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this header is used by sensors and audio daemons.

Copy link
Contributor

Choose a reason for hiding this comment

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

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/HAP_farf.h
Copy link
Contributor

Choose a reason for hiding this comment

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

This one also looks like an internal header, rather than something that should be used by library users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see this is included by multiple clients like sensors, CV etc.

fastrpc_include_HEADERS += $(top_srcdir)/inc/HAP_pls.h
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 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/HAP_debug.h
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a debug for adsprpcd or for user apps?

fastrpc_include_HEADERS += $(top_srcdir)/inc/dspqueue.h
fastrpc_include_HEADERS += $(top_srcdir)/inc/adsp_default_listener1.h
Copy link
Contributor

Choose a reason for hiding this comment

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

Which of the headers actually define an API that is useable by the users of fastrpc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

adsp_default_listener_register is not documented at all. What is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add documentation for the header as a separate commit.

Loading