931655 - [HW test] Simple FM app implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Tom Jao [:mbrsl]]

We need a simple FM app for testing purpose. This also makes sure that not only official FM app works well on webAPI, but also third-party apps.

Comment 1

[Tom Jao [:mbrsl]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/14556

I found a bug that radio.cancelSeek() doesn't work properly. In this test, I first call seekUp() then call cancelSeek(). cancelSeek() reports a successful cancellation but the frequency is still being changed.
If the code in this test is OK, I'll file a bug of cancelSeek().

Comment 2

[Tom Jao [:mbrsl]]

Fred, can you give me some advice of this test? Or do you know who is familiar with FMRadio API? I'm thinking if there are any better ways to test FMRadio API, thank you.

Comment 3

[[e-mail]]

Hi Pin, any idea of radio.cancelSeek() doesn't work properly?

Comment 4

[Pin Zhang [:pzhang] (inactive)]

(In reply to Fred Lin [:gasolin] from comment #3)
> Hi Pin, any idea of radio.cancelSeek() doesn't work properly?

I think the behaviour Tom described in comment 1 is expected. When mozFMRadio.seekUp() is called, the SEEK command is sent to FM radio HW, the FM radio chip then starts to check frequency step by step (I guess that's why we need to configure the chip with an appropriate channel width, say 100kHz or 200kHz), until a strong signal is found, which means the frequency is keeping changed when searching, so if we cancelled it before an available channel is found, the frequency will stop at some frequency between the frequency you started to search and next available one.

Comment 5

[[e-mail]]

Thanks for clarification! 

Tom, you can ask pin for review once patch is ready.

Comment 6

[Tom Jao [:mbrsl]]

(In reply to Pin Zhang [:pzhang] from comment #4)
> I think the behaviour Tom described in comment 1 is expected. When
> mozFMRadio.seekUp() is called, the SEEK command is sent to FM radio HW, the
> FM radio chip then starts to check frequency step by step (I guess that's
> why we need to configure the chip with an appropriate channel width, say
> 100kHz or 200kHz), until a strong signal is found, which means the frequency
> is keeping changed when searching, so if we cancelled it before an available
> channel is found, the frequency will stop at some frequency between the
> frequency you started to search and next available one.

Thank you for the explanation.

Though I call cancelSeek() after seekUp(), it always stops at the next correct frequency rather than a 'midway' frequency. The result of seekUp()+cancelSeek() is the same of only seekUP(). Maybe seekUP() runs too fast and is almost finished before cancelSeek() reaches HW. I have no idea whether this is true and come up with a better way to test it.

Comment 7

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

Hi Tom, please check my comments on Github. I didn't see any assertion, so it's not the usual *tests* I'm familiar with, then I have one silly question, what's the purpose of this bug?

Comment 8

[Pin Zhang [:pzhang] (inactive)]

(In reply to Tom Jao [:mbrsl] from comment #6)
> (In reply to Pin Zhang [:pzhang] from comment #4)
> > I think the behaviour Tom described in comment 1 is expected. When
> > mozFMRadio.seekUp() is called, the SEEK command is sent to FM radio HW, the
> > FM radio chip then starts to check frequency step by step (I guess that's
> > why we need to configure the chip with an appropriate channel width, say
> > 100kHz or 200kHz), until a strong signal is found, which means the frequency
> > is keeping changed when searching, so if we cancelled it before an available
> > channel is found, the frequency will stop at some frequency between the
> > frequency you started to search and next available one.
> 
> Thank you for the explanation.
> 
> Though I call cancelSeek() after seekUp(), it always stops at the next
> correct frequency rather than a 'midway' frequency. The result of
> seekUp()+cancelSeek() is the same of only seekUP(). Maybe seekUP() runs too
> fast and is almost finished before cancelSeek() reaches HW. I have no idea
> whether this is true and come up with a better way to test it.

Maybe, could you check the status of the two requests? If seekUp().onsuccess fired, then cancelSeek().onerror should be called, vice vesa. We have the similar tests for this:
  http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/test_cancel_seek.js#24

Comment 9

[Tom Jao [:mbrsl]]

(In reply to Pin Zhang [:pzhang] from comment #7)
> Comment on attachment 8345731 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/14556
> 
> Hi Tom, please check my comments on Github. I didn't see any assertion, so
> it's not the usual *tests* I'm familiar with, then I have one silly
> question, what's the purpose of this bug?
This is a subtest in a manual testing app called 'UI tests'. For FMRadio, I think it's much simpler to write a test for human than a test for machine because you need to check the sound.

Comment 10

[Tom Jao [:mbrsl]]

(In reply to Pin Zhang [:pzhang] from comment #8)
> (In reply to Tom Jao [:mbrsl] from comment #6)
> > (In reply to Pin Zhang [:pzhang] from comment #4)
> > > I think the behaviour Tom described in comment 1 is expected. When
> > > mozFMRadio.seekUp() is called, the SEEK command is sent to FM radio HW, the
> > > FM radio chip then starts to check frequency step by step (I guess that's
> > > why we need to configure the chip with an appropriate channel width, say
> > > 100kHz or 200kHz), until a strong signal is found, which means the frequency
> > > is keeping changed when searching, so if we cancelled it before an available
> > > channel is found, the frequency will stop at some frequency between the
> > > frequency you started to search and next available one.
> > 
> > Thank you for the explanation.
> > 
> > Though I call cancelSeek() after seekUp(), it always stops at the next
> > correct frequency rather than a 'midway' frequency. The result of
> > seekUp()+cancelSeek() is the same of only seekUP(). Maybe seekUP() runs too
> > fast and is almost finished before cancelSeek() reaches HW. I have no idea
> > whether this is true and come up with a better way to test it.
> 
> Maybe, could you check the status of the two requests? If seekUp().onsuccess
> fired, then cancelSeek().onerror should be called, vice vesa. We have the
> similar tests for this:
>  
> http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/
> test_cancel_seek.js#24
Yes, I checked it. I got the events as following.

call seekUP()
call cancelSeek()
seekUP().onerror: 'Seek: action is cancelled'
canelSeek().onsuccess
FMRadio.onfrequencychange

This seems to be a normal flow but the resulting frequency is 'correct' rather than 'midway' after FMRadio.onfrequencychange.


For another situation that seekup's onsuccess fires before cancelSeek's onerror, I think the result is fine.

Comment 11

[Tom Jao [:mbrsl]]

Comment on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

Fixed, thank for the first review.

Comment 12

[Pin Zhang [:pzhang] (inactive)]

(In reply to Tom Jao [:mbrsl] from comment #10)
> (In reply to Pin Zhang [:pzhang] from comment #8)
> > (In reply to Tom Jao [:mbrsl] from comment #6)
> > > (In reply to Pin Zhang [:pzhang] from comment #4)
> > > > I think the behaviour Tom described in comment 1 is expected. When
> > > > mozFMRadio.seekUp() is called, the SEEK command is sent to FM radio HW, the
> > > > FM radio chip then starts to check frequency step by step (I guess that's
> > > > why we need to configure the chip with an appropriate channel width, say
> > > > 100kHz or 200kHz), until a strong signal is found, which means the frequency
> > > > is keeping changed when searching, so if we cancelled it before an available
> > > > channel is found, the frequency will stop at some frequency between the
> > > > frequency you started to search and next available one.
> > > 
> > > Thank you for the explanation.
> > > 
> > > Though I call cancelSeek() after seekUp(), it always stops at the next
> > > correct frequency rather than a 'midway' frequency. The result of
> > > seekUp()+cancelSeek() is the same of only seekUP(). Maybe seekUP() runs too
> > > fast and is almost finished before cancelSeek() reaches HW. I have no idea
> > > whether this is true and come up with a better way to test it.
> > 
> > Maybe, could you check the status of the two requests? If seekUp().onsuccess
> > fired, then cancelSeek().onerror should be called, vice vesa. We have the
> > similar tests for this:
> >  
> > http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/
> > test_cancel_seek.js#24
> Yes, I checked it. I got the events as following.
> 
> call seekUP()
> call cancelSeek()
> seekUP().onerror: 'Seek: action is cancelled'
> canelSeek().onsuccess
> FMRadio.onfrequencychange
> 
> This seems to be a normal flow but the resulting frequency is 'correct'
> rather than 'midway' after FMRadio.onfrequencychange.
> 

According to your codes, the initial frequency is the frequencyLowerBound, i.e. 87.5MHz right? Then what frequency it stopped at?

Comment 13

[Pin Zhang [:pzhang] (inactive)]

(In reply to Tom Jao [:mbrsl] from comment #9)
> (In reply to Pin Zhang [:pzhang] from comment #7)
> > Comment on attachment 8345731 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/14556
> > 
> > Hi Tom, please check my comments on Github. I didn't see any assertion, so
> > it's not the usual *tests* I'm familiar with, then I have one silly
> > question, what's the purpose of this bug?
> This is a subtest in a manual testing app called 'UI tests'. For FMRadio, I
> think it's much simpler to write a test for human than a test for machine
> because you need to check the sound.

If it's just for manually testing, why don't use the FM radio app?

Comment 14

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

Please check my comments again. I didn't comment a lot about the UI except the text content of the switch button, because it might really confuse the tester.

Comment 15

[[e-mail]]

This case is used for Manufacturer automatic testing. 
They might have an machine that emulate human finger behaviors and tend to test all cases within engineering mode app.

Comment 16

[Tom Jao [:mbrsl]]

(In reply to Pin Zhang [:pzhang] from comment #12)
> (In reply to Tom Jao [:mbrsl] from comment #10)
> > (In reply to Pin Zhang [:pzhang] from comment #8)
> > > Maybe, could you check the status of the two requests? If seekUp().onsuccess
> > > fired, then cancelSeek().onerror should be called, vice vesa. We have the
> > > similar tests for this:
> > >  
> > > http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/
> > > test_cancel_seek.js#24
> > Yes, I checked it. I got the events as following.
> > 
> > call seekUP()
> > call cancelSeek()
> > seekUP().onerror: 'Seek: action is cancelled'
> > canelSeek().onsuccess
> > FMRadio.onfrequencychange
> > 
> > This seems to be a normal flow but the resulting frequency is 'correct'
> > rather than 'midway' after FMRadio.onfrequencychange.
> > 
> 
> According to your codes, the initial frequency is the frequencyLowerBound,
> i.e. 87.5MHz right? Then what frequency it stopped at?

It stopped at 88.9 which is an valid frequency for a radio station in my region. A single radio.seekUP() stopped exactly the same at 88.9.

Comment 17

[Tom Jao [:mbrsl]]

(In reply to Pin Zhang [:pzhang] from comment #14)
> Comment on attachment 8345731 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/14556
> 
> Please check my comments again. I didn't comment a lot about the UI except
> the text content of the switch button, because it might really confuse the
> tester.
Sorry for the very late reply. I was away for personal reasons and vacation.
I fixed these issues.

(In reply to Fred Lin [:gasolin] from comment #15)
> This case is used for Manufacturer automatic testing. 
> They might have an machine that emulate human finger behaviors and tend to
> test all cases within engineering mode app.
Thank you for the explanation, Fred :)

Comment 18

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

You are asking *me* instead of [e-mail] for review, right? :)

Comment 19

[Tom Jao [:mbrsl]]

Oops, I chose the wrong email, sorry.

Comment 20

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

Please address the nits as I commented, then r=me.

Comment 21

[[e-mail]]

merged in gaia-master https://github.com/mozilla-b2g/gaia/commit/1f6e2e53cc2b41e8008a7905c72eb20fc3af52c4

thanks!

Comment 22

[Pin Zhang [:pzhang] (inactive)]

(In reply to Fred Lin [:gasolin] from comment #21)
> merged in gaia-master
> https://github.com/mozilla-b2g/gaia/commit/
> 1f6e2e53cc2b41e8008a7905c72eb20fc3af52c4
> 
> thanks!

Hi Fred, as I said in comment 20, Tom need to fix the nits I commented.

Comment 23

[Tom Jao [:mbrsl]]

I fixed the nits before asking Fred for merge. Sorry that I didn't leave this message on Bugzilla.

Comment 24

[Pin Zhang [:pzhang] (inactive)]

(In reply to Tom Jao [:mbrsl] from comment #23)
> I fixed the nits before asking Fred for merge. Sorry that I didn't leave
> this message on Bugzilla.

But line4-15 should be merged with line19-30, the commit[1] didn't do that.

[1] https://github.com/mozilla-b2g/gaia/commit/1f6e2e53cc2b41e8008a7905c72eb20fc3af52c4

Comment 25

[[e-mail]]

reverted 4abe805b61018fbe5207904eb94bfb177b596ff9

Comment 26

[[e-mail]]

Tom, please provide another PR to fix it

Comment 27

[Tom Jao [:mbrsl]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15101

*embarrassed*

Comment 28

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8357070 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15101

r=me, but there is still one styling nit need to be fixed, thanks, Tom.

Comment 29

[Tom Jao [:mbrsl]]

(In reply to Pin Zhang [:pzhang] from comment #28)
> Comment on attachment 8357070 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/15101
> 
> r=me, but there is still one styling nit need to be fixed, thanks, Tom.

PR Updated.

Comment 30

[[e-mail]]

merged to gaia-master https://github.com/mozilla-b2g/gaia/commit/34e46e7f16f19623f652dd13bc56019603cc3316