Closed Bug 931655 Opened 8 years ago Closed 8 years ago

[HW test] Simple FM app implementation

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tjao, Assigned: tjao)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → tjao
Blocks: eng-mode
Component: Gaia → Gaia::TestAgent
Component: Gaia::TestAgent → Gaia
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().
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.
Flags: needinfo?(gasolin)
Hi Pin, any idea of radio.cancelSeek() doesn't work properly?
Flags: needinfo?(gasolin) → needinfo?(pzhang)
(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.
Flags: needinfo?(pzhang)
Thanks for clarification! 

Tom, you can ask pin for review once patch is ready.
Attachment #8345731 - Flags: review?(pzhang)
(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 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?
Attachment #8345731 - Flags: review?(pzhang) → review-
(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
(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.
(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 on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

Fixed, thank for the first review.
Attachment #8345731 - Flags: review- → review?(pzhang)
(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?
(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 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.
Attachment #8345731 - Flags: review?(pzhang) → review-
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.
(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.
(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 :)
Attachment #8345731 - Flags: review- → review?(pzhang)
Comment on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

You are asking *me* instead of pzhang@cse.unl.edu for review, right? :)
Attachment #8345731 - Flags: review?(pzhang) → review?(pzhang)
Oops, I chose the wrong email, sorry.
Comment on attachment 8345731 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14556

Please address the nits as I commented, then r=me.
Attachment #8345731 - Flags: review?(pzhang) → review+
merged in gaia-master https://github.com/mozilla-b2g/gaia/commit/1f6e2e53cc2b41e8008a7905c72eb20fc3af52c4

thanks!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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.
I fixed the nits before asking Fred for merge. Sorry that I didn't leave this message on Bugzilla.
(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
reverted 4abe805b61018fbe5207904eb94bfb177b596ff9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Tom, please provide another PR to fix it
*embarrassed*
Attachment #8345731 - Attachment is obsolete: true
Attachment #8357070 - Flags: review?(pzhang)
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.
Attachment #8357070 - Flags: review?(pzhang) → review+
(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.
merged to gaia-master https://github.com/mozilla-b2g/gaia/commit/34e46e7f16f19623f652dd13bc56019603cc3316
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.