Closed Bug 1041085 Opened 6 years ago Closed 5 years ago

FM RDS support in HAL

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch RDS support for HAL FM Radio (obsolete) — Splinter Review
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.
Attachment #8459077 - Flags: review?(dhylands)
Updated to use standard v4l2 controls for rds.
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.
Attachment #8459077 - Flags: review?(dhylands) → review+
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.
Attached patch RDS support for HAL FM Radio, v2 (obsolete) — Splinter Review
Review comments addressed.
Attachment #8459077 - Attachment is obsolete: true
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.
Attachment #8459771 - Attachment is obsolete: true
Attached patch RDS support for HAL FM Radio, v3 (obsolete) — Splinter Review
Unbitrotted
Attachment #8459913 - Attachment is obsolete: true
https://hg.mozilla.org/integration/b2g-inbound/rev/d1ad5d5bc00f

I'll file a separate bug for adding RDS support to the flame kernel.
No longer blocks: 1064660
Backed out -

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

ICS doesn't have sufficiently new headers to support RDS.
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.
Attachment #8471904 - Attachment is obsolete: true
A followup to fix non-unified build bustage -

https://hg.mozilla.org/integration/b2g-inbound/rev/bd5772307510
https://hg.mozilla.org/mozilla-central/rev/fa48349c7945
https://hg.mozilla.org/mozilla-central/rev/bd5772307510
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Bug 1077090 opened to add RDS support to the flame driver.
Depends on: 1077090
You need to log in before you can comment on or make changes to this bug.