-
Notifications
You must be signed in to change notification settings - Fork 54
Refactor: Generalize Code by Moving Vendor and Android-Specific Elements #154
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
base: development
Are you sure you want to change the base?
Conversation
5ad182f to
e92d2c8
Compare
|
Please check if you can break the change into multiple commits instead of a single commit. |
e92d2c8 to
66cba44
Compare
|
This pull request has been marked as stale due to 60 days of inactivity. To prevent automatic closure in 10 days, remove the stale label or add a comment. You can reopen a closed pull request at any time. |
|
@quic-vkatoch, can you please rebase this PR? |
0ffa6c4 to
f7d23d7
Compare
f7d23d7 to
4e34aac
Compare
Done |
4e34aac to
44fd420
Compare
Head branch was pushed to by a user without write access
ef69f29 to
040ef4c
Compare
| #endif | ||
| #endif /* ENABLE_UPSTREAM_DRIVER_INTERFACE */ | ||
|
|
||
| #ifndef VENDOR_DSP_LOCATION |
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.
How are these two changes related to each other?
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.
These changes are unrelated; I will update the PR.
| #ifndef FASTRPC_TRACE_H | ||
| #define FASTRPC_TRACE_H | ||
|
|
||
| #if ((defined _ANDROID) || (defined ANDROID)) || (defined DISABLE_ATRACE) && !defined(LE_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.
Again, totally unrelated changes
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.
These changes are unrelated; I will update the PR.
|
|
||
| PL_DEP(gpls) | ||
| PL_DEP(listener_android) | ||
| PL_DEP(listener) |
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.
Sould this be a part of the previous commit?
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.
You're right, I'll do that.
| dspqueue/dspqueue_cpu.c \ | ||
| dspqueue/dspqueue_rpc_stub.c \ | ||
| listener_android.c \ | ||
| listener.c \ |
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.
Sould this be a part of the previous commit?
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.
You're right, I'll do that.
| #include "fastrpc_pm.h" | ||
| #include "fastrpc_procbuf.h" | ||
| #include "listener_android.h" | ||
| #include "listener.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.
Ugh. Make sure that each commit works on its own. The code was broken up to now.
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.
Sure, I'll do that.
| pthread_mutex_destroy(&hlist[i].async_init_deinit_mut); | ||
| } | ||
| listener_android_deinit(); | ||
| listener_deinit(); |
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.
Yuck
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.
I will update the PR.
| const char *format, ...) { | ||
| int len = 0; | ||
| va_list argp; | ||
| char *buf = NULL, *log = NULL; |
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.
Again, feels like several unrelated changed squashed toggether.
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.
When I removed debug_build_type (ro.debuggable) under fastrpc_log_init, it caused issues due to incorrectness in the HAP_debug_runtime function, so I had to update it. I'll remove this change from the current PR and submit a separate PR to address the fix.
040ef4c to
e26a068
Compare
|
This PR depends on PR #252 being merged first. |
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.
If we are removing the support for /vendor/dsp, where is shell expected to be pushed for android targets ? if it is /usr/lib/dsp/ like other LE targets, is /usr partition available on android targets ?
how is this change tested on android targets ?
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.
I have to check where the shell should be placed if /vendor/dsp support is removed, or if we actually need to remove it in the first place.
Regarding testing: earlier, the daemons were already running (opened the shell) and the tests passed. After killing and updating with new daemons, the tests are now failing. I will check this further and update.
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 shell is loaded from the same folder as all other DSP libraries. What's the question?
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.
If we are removing the support for /vendor/dsp, where is shell expected to be pushed for android targets ? if it is /usr/lib/dsp/ like other LE targets, is /usr partition available on android targets ?
All DSP binaries are located in the subdirs of /usr/share/qcom, one subdir per device.
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.
On Android systems you can reuse the same approach, changing /usr/share/qcom/ to /vendor or its subdir. All config files should be relative to that path,
Create fastrpc_android.h/c for Android-specific DSP search paths and update fastrpc_config_parser.h for Linux paths, removing vendor directories from both. Remove VENDOR_DSP_LOCATION and VENDOR_DOM_LOCATION macros from apps_std_internal.h. Simplify open_shell() in fastrpc_apps_user.c by removing vendor path fallback logic. Update Makefiles to conditionally compile fastrpc_android.c for Android builds. This separates platform-specific configurations and eliminates redundant vendor path fallbacks. Signed-off-by: Vinayak Katoch <vkatoch@qti.qualcomm.com>
Change header guard macros from FASTRPC_ANDROID_USER_H to FASTRPC_USER_H to remove Android-specific naming and make the header more generic for cross-platform usage. Signed-off-by: Vinayak Katoch <vkatoch@qti.qualcomm.com>
Rename listener_android.h to listener.h and listener_android.c to listener.c to remove Android-specific naming. Update all function names from listener_android_* to listener_* pattern. Update header guards, includes, Makefile entries, platform library dependencies, and all function calls throughout the codebase to maintain compilation compatibility. Signed-off-by: Vinayak Katoch <vkatoch@qti.qualcomm.com>
Remove unused convert_level_to_android_priority() function, TODO comments referencing Android-specific code, and Android include statements. Update AEE_EINVHANDLE comment to remove vendor-specific reference and clean up conditional compilation blocks for Android platforms. Signed-off-by: Vinayak Katoch <vkatoch@qti.qualcomm.com>
e26a068 to
eb9d72f
Compare
Move Android property arrays (ANDROIDP_DEBUG_VAR_NAME and ANDROID_DEBUG_VAR_NAME) to fastrpc_android.c. Add functions android_get_property_int(), android_get_property_string(), and android_is_debug_build() to handle Android-specific property operations and build type checking. Update fastrpc_apps_user.c to call Android functions when on Android platform while maintaining environment variable fallback. Update fastrpc_log.c to use android_is_debug_build() for debug buffer allocation. Android functionality preserved and isolated in dedicated files. Signed-off-by: Vinayak Katoch <vkatoch@qti.qualcomm.com>
eb9d72f to
056c191
Compare
Refactored the codebase by moving vendor-specific and Android-specific elements into dedicated files, making the project more generic.