1041085 - FM RDS support in HAL

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Michael Wu [:mwu]]

Attachment: RDS support for HAL FM Radio

This adds support for reading RDS groups through a HAL API. The V4L2 RDS api http://linuxtv.org/downloads/v4l-dvb-apis/rds.html is used to read individual RDS blocks. A dedicated thread on the HAL side reads this data and attempts to assemble the blocks into whole RDS groups.

For more information about RDS, look for the RBDS standard.

Comment 1

[Michael Wu [:mwu]]

Attachment: Raw RDS support for Flame kernel, v4

Updated to use standard v4l2 controls for rds.

Comment 2

[Dave Hylands [:dhylands]]

Comment on attachment 8459077 [details] [diff] [review]
RDS support for HAL FM Radio

Review of attachment 8459077 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly minor stuff.

I think that there is at least one bug (errno != EINTR) and it looks like you're leaking handles, and the rds observer list.

r=me with stuff addressed.

::: hal/Hal.cpp
@@ +1014,1 @@
>      ClearOnShutdown(&sFMRadioObservers);

Shouldn't you have a ClearOnShutdown for sFMRadioRDSObservers as well?

::: hal/gonk/GonkFMRadio.cpp
@@ +503,5 @@
>  {}
>  
> +/* Runs on the rds thread */
> +static void *
> +readRDSData(void *data)

nit: I think that this should have the word Thread in it.
nit: void* data

@@ +512,5 @@
> +  int pipefd = (int)data;
> +
> +  ScopedClose epollfd(epoll_create(2));
> +  if (epollfd < 0) {
> +    HAL_LOG(("Could not create epoll FD for RDS thread"));

nit: This (and the other similar error exits) should probably at least print errno.

@@ +537,5 @@
> +  epoll_event events[2] = {{ 0 }};
> +  int event_count;
> +  uint32_t block_bitmap = 0;
> +  while ((event_count = epoll_wait(epollfd, events, 2, -1)) > 0 ||
> +         errno != EINTR) {

Is errno != EINTR correct?

It seems it will exit the while loop if a signal is received, but keep running on error?

I would have thought you'd want to use errno == EINTR (so it will hit the continue on line 555)

@@ +541,5 @@
> +         errno != EINTR) {
> +    bool RDSDataAvailable = false;
> +    for (int i = 0; i < event_count; i++) {
> +      if (events[i].data.fd == pipefd) {
> +        char tmp[32];

This is being used to signal that it should quit right?

Why not just arbitrarily return nullptr and not bother reading the data since we're going to throw it away anyways?

@@ +565,5 @@
> +    int lastblock = -1;
> +    for (int i = 0; i < blockcount; i++) {
> +      if ((rdsblocks[i].block & V4L2_RDS_BLOCK_MSK) == V4L2_RDS_BLOCK_INVALID ||
> +           rdsblocks[i].block & V4L2_RDS_BLOCK_ERROR) {
> +        block_bitmap |= 1 << 4;

4 seems to correspond to V4L2_RDS_BLOCK_C_ALT

Shouldn't we use a symbolic constant here?

Naively, I would suggest V4L2_RDS_BLOCK_ERROR (0x80) (which is the same as 1 << V4L2_RDS_BLOCK_INVALID)

@@ +579,5 @@
> +      // However, we only process whole RDS groups, so it doesn't matter here.
> +      if (blocknum == V4L2_RDS_BLOCK_C_ALT)
> +        blocknum = V4L2_RDS_BLOCK_C;
> +      if (blocknum > V4L2_RDS_BLOCK_D) {
> +        HAL_LOG(("Unexpected RDS block number. This is a driver bug."));

nit: print the unexpected block number

@@ +589,5 @@
> +
> +      // Skip the group if we skipped a block.
> +      // This stops us from processing blocks sent out of order.
> +      if (block_bitmap != ((1 << blocknum) - 1))
> +        block_bitmap |= 1 << 4;

Shouldn't this have a continue? It doesn't seem like performing any other processing is useful.

@@ +596,5 @@
> +
> +      lastblock = blocknum;
> +      blocks[blocknum] = (rdsblocks[i].msb << 8) | rdsblocks[i].lsb;
> +
> +      if (block_bitmap != 0x0F)

This is really A | B | C | D right? Since there isn't a constant, perhaps just add a comment to explain where the 0x0F comes from.

@@ +615,5 @@
> +static int sRDSPipeFD;
> +
> +void
> +EnableRDS(uint32_t aMask)
> +{

nit: shouldn't this have AssertMainThread() ?

@@ +656,5 @@
> +}
> +
> +void
> +DisableRDS()
> +{

nit: shouldn't this have AssertMainThread() ?

@@ +668,5 @@
> +  sRDSEnabled = false;
> +
> +  write(sRDSPipeFD, "x", 1);
> +
> +  pthread_join(sRDSThread, nullptr);

It looks like you're leaking the read and write pipe handles. I don't see them getting closed anywhere during the success case.

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 8459077 [details] [diff] [review]
RDS support for HAL FM Radio

Review of attachment 8459077 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/Hal.cpp
@@ +1014,1 @@
>      ClearOnShutdown(&sFMRadioObservers);

Good catch. I remember thinking about this, and apparently never got around to it.

::: hal/gonk/GonkFMRadio.cpp
@@ +503,5 @@
>  {}
>  
> +/* Runs on the rds thread */
> +static void *
> +readRDSData(void *data)

Done

@@ +512,5 @@
> +  int pipefd = (int)data;
> +
> +  ScopedClose epollfd(epoll_create(2));
> +  if (epollfd < 0) {
> +    HAL_LOG(("Could not create epoll FD for RDS thread"));

Done

@@ +537,5 @@
> +  epoll_event events[2] = {{ 0 }};
> +  int event_count;
> +  uint32_t block_bitmap = 0;
> +  while ((event_count = epoll_wait(epollfd, events, 2, -1)) > 0 ||
> +         errno != EINTR) {

Fixed. Good catch.

@@ +541,5 @@
> +         errno != EINTR) {
> +    bool RDSDataAvailable = false;
> +    for (int i = 0; i < event_count; i++) {
> +      if (events[i].data.fd == pipefd) {
> +        char tmp[32];

I probably didn't want to risk blocking the writer. But that seems pretty unlikely, so I've switched to returning early.

@@ +565,5 @@
> +    int lastblock = -1;
> +    for (int i = 0; i < blockcount; i++) {
> +      if ((rdsblocks[i].block & V4L2_RDS_BLOCK_MSK) == V4L2_RDS_BLOCK_INVALID ||
> +           rdsblocks[i].block & V4L2_RDS_BLOCK_ERROR) {
> +        block_bitmap |= 1 << 4;

I switched to V4L2_RDS_BLOCK_INVALID.

@@ +579,5 @@
> +      // However, we only process whole RDS groups, so it doesn't matter here.
> +      if (blocknum == V4L2_RDS_BLOCK_C_ALT)
> +        blocknum = V4L2_RDS_BLOCK_C;
> +      if (blocknum > V4L2_RDS_BLOCK_D) {
> +        HAL_LOG(("Unexpected RDS block number. This is a driver bug."));

Done.

@@ +589,5 @@
> +
> +      // Skip the group if we skipped a block.
> +      // This stops us from processing blocks sent out of order.
> +      if (block_bitmap != ((1 << blocknum) - 1))
> +        block_bitmap |= 1 << 4;

Sure.

@@ +596,5 @@
> +
> +      lastblock = blocknum;
> +      blocks[blocknum] = (rdsblocks[i].msb << 8) | rdsblocks[i].lsb;
> +
> +      if (block_bitmap != 0x0F)

Comment added.

@@ +615,5 @@
> +static int sRDSPipeFD;
> +
> +void
> +EnableRDS(uint32_t aMask)
> +{

I guess, but nothing else in this file or directory uses AssertMainThread. Seems like that's only used at the top level. There's a lot of functions to add AssertMainThread to if we want to start doing that.

@@ +668,5 @@
> +  sRDSEnabled = false;
> +
> +  write(sRDSPipeFD, "x", 1);
> +
> +  pthread_join(sRDSThread, nullptr);

I really should've looked over this patch more closely. I've added ScopedClose to the rds thread to cover the read pipe and a close() here to close the write pipe.

Comment 4

[Michael Wu [:mwu]]

Attachment: RDS support for HAL FM Radio, v2

Review comments addressed.

Comment 5

[Michael Wu [:mwu]]

Attachment: Raw RDS support for Flame kernel, v5

Turns out we can't rely on epoll/poll/select if we don't implement the poll fop to tell the kernel when there's data to be read.

Comment 6

[Michael Wu [:mwu]]

Attachment: RDS support for HAL FM Radio, v3

Unbitrotted

Comment 7

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d1ad5d5bc00f

I'll file a separate bug for adding RDS support to the flame kernel.

Comment 8

[Michael Wu [:mwu]]

Backed out -

https://hg.mozilla.org/integration/b2g-inbound/rev/d1ad5d5bc00f

ICS doesn't have sufficiently new headers to support RDS.

Comment 9

[Michael Wu [:mwu]]

https://tbpl.mozilla.org/?tree=Try&rev=cae1e77fe413

Comment 10

[Michael Wu [:mwu]]

Attachment: RDS support for HAL FM Radio, v4

The logging macro changed, so I had to update that. Also had to add some defines and a struct as a fallback for devices without a sufficiently new videodev2.h.

Comment 11

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fa48349c7945

Comment 12

[Michael Wu [:mwu]]

A followup to fix non-unified build bustage -

https://hg.mozilla.org/integration/b2g-inbound/rev/bd5772307510

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa48349c7945
https://hg.mozilla.org/mozilla-central/rev/bd5772307510

Comment 14

[Michael Wu [:mwu]]

Bug 1077090 opened to add RDS support to the flame driver.