CirrusLogic / tinyhal

Configurable audio HAL for Android
Apache License 2.0
32 stars 17 forks source link

Build failure on 5.1.1_r18 #1

Closed mfuzzey closed 8 years ago

mfuzzey commented 9 years ago

Current head (9c579fc76db5e975e9cf88123351bacf8299a627) fails to build on Lollipop 5.1.1_r18 with:

1) external/tinyhal/audio/audio_hw.c:1036:13: error: passing argument 2 of 'compress_get_tstamp' from incompatible pointer type [-Werror] if (compress_get_tstamp(out->compress, &samples, &sampling_rate) == 0) { ^ In file included from external/tinyhal/audio/audio_hw.c:45:0: external/tinycompress/include/tinycompress/tinycompress.h:127:5: note: expected 'long unsigned int ' but argument is of type 'unsigned int ' int compress_get_tstamp(struct compress *compress,

2) external/tinyhal/configmgr/audio_config.c: In function 'ctl_open': external/tinyhal/configmgr/audio_config.c:315:9: error: implicit declaration of function 'mixer_update_ctls' [-Werror=implicit-function-declaration] mixer_update_ctls(cm->mixer); ^

3) external/tinyhal/configmgr/audio_config.c:364:5: error: implicit declaration of function 'mixer_ctl_get_id' [-Werror=implicit-function-declaration] pctl->id = mixer_ctl_get_id(ctl); ^

1) Is trivially fixed by:

diff --git a/audio/audio_hw.c b/audio/audio_hw.c
index 839955f..3d5be30 100644
--- a/audio/audio_hw.c
+++ b/audio/audio_hw.c
@@ -1019,7 +1019,7 @@ static int out_compress_get_render_position(const struct audio_stream_out *strea
                                    uint32_t *dsp_frames)
 {
     struct stream_out_compress *out = (struct stream_out_compress *)stream;
-    unsigned int samples;
+    long unsigned int samples;
     unsigned int sampling_rate;

     if (dsp_frames) {

For 3) the missing function mixer_ctl_get_id() is in the upstream tinyalsa but not in Google's version

However for 2) the missing function mixer_update_ctls() is not upstream either - are you using a custom version??

rfvirgil commented 9 years ago

I need to update the readme to say that TinyHAL is built against the latest upstream code.

1) In the current upstream tinycompress code the arguments are unsigned int *. See http://git.alsa-project.org/?p=tinycompress.git;a=blob;f=include/tinycompress/tinycompress.h;h=68626a42e4e7b909ac5ada9fb6176676ca393af6;hb=refs/heads/master

If you think the mainline tinycompress code is wrong please send a patch to vinod.koul@intel.com and cc alsa-devel@alsa-project.org

2) mixer_update_ctls() is in the open pull requests for upstream tinyalsa https://github.com/tinyalsa/tinyalsa/pull/54

3) As you say, the function is in the upstream tinyalsa code.

rfvirgil commented 9 years ago

I have updated the README file to explain that TinyHAL builds against the upstream versions of tinyalsa and tincompress, and links to where to get that source.

rfvirgil commented 8 years ago

I'm closing this as it was really a lack of documentation and I've now added the necessary information into the README.

HighwayStar commented 7 years ago

Upstream tinyalsa still incompatible with tinyhal. Are there any way to build tinyhal with upstream or better google tinyalsa?

aleksander0m commented 7 years ago

Yeah, it's very very unfortunate that tinyhal git master doesn't build against tinyalsa git master (yet). Depending on an open pull request is just not right... maybe the documentation should state that this tinyhal builds against some other tinyalsa repository (@rfvirgil's?), just not upstream tinyalsa git master.

aleksander0m commented 7 years ago

Looks like mixer_update_ctls() was renamed as mixer_add_new_ctls() before getting merged to tinyalsa?

rfvirgil commented 7 years ago

On Sat, 2017-07-29 at 01:37 -0700, Aleksander Morgado wrote:

Yeah, it's very very unfortunate that tinyhal git master doesn't build against tinyalsa git master (yet). Depending on an open pull request is just not right... maybe the documentation should state that this tinyhal builds against some other tinyalsa repository (@rfvirgil's?), just not upstream tinyalsa git master.

I'll do a patch to TinyHAL at some point. What happened here is that I made a patch to tinyalsa to add mixer_update_ctls() and to TinyHAL to use it because there were people asking us for the functionality it provides. However, the tinyalsa patch got stuck in review for a very long time (about 2 years I think) because the tinyalsa maintainer didn't have time to work on tinyalsa and during that time people were taking the patch manually into their tinyalsa trees because they wanted to use it. We've had to live with this unsatisfactory situation of trees not being in sync because of the people who were needing the new functionality.

The last time I rebased the tinyalsa patch for review someone else had added a function with a very similar name that did a completely different task so I renamed my function to avoid confusion. Trouble is there's people out there using TinyHAL with their codebase using the previous version of the patch so I now have another sync problem and I've been favouring not breaking existing users of TinyHAL that I know about until everyone can sync up. Also for a while we couldn't get the tinyalsa master code to build and weren't able to figure out exactly what needed patching to fix it, which made testing TinyHAL against it rather difficult.