• News
  • Idefisk
  • Tools
  • Tutorials
  • Forum
  • Reviews
  • VoIP Providers
  • Archives
  • Gallery
ZOIPER softphone
AsteriskGuru Archives
Mailing List Archives
 
 FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

[asterisk-dev] [Code Review] hints with 2+ devices that incl

 
   AsteriskGuru Archives Forum Index -> Asterisk-Dev
View previous topic :: View next topic  
Author Message
dvossel at digium.com
Guest





PostPosted: Tue May 26, 2009 5:09 pm    Post subject: [asterisk-dev] [Code Review] hints with 2+ devices that incl

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/
-----------------------------------------------------------

Review request for Asterisk Developers.


Summary
-------

This is a community patch I cleaned up and tested. Below is the reporter's, p_lidheimer, description of the issue pulled directly from issue #15057.

----------------------------
If you have a hint with two (or more) devices or a device and a custom devstate that includes ONHOLD, you get unexpected results. Two examples:

exten => 222,hint,SIP/222&SIP/211
exten => 333,hint,Custom:DND333&SIP/333

The following results are inconsistent as they should all return ONHOLD:

ONHOLD & IDLE = IDLE
ONHOLD & UNAVAILABLE => UNAVAILABLE
ONHOLD & UNKNOWN => IDLE
ONHOLD & INVALID => IDLE

Per Russell Bryant's response on the mailing list:

"I would agree that this is a bug. I would expect that combination of
states to result in an OnHold state."


This addresses bug 15057.
https://issues.asterisk.org/view.php?id=15057


Diffs
-----

/trunk/include/asterisk/devicestate.h 196415
/trunk/main/devicestate.c 196415

Diff: http://reviewboard.digium.com/r/254/diff


Testing
-------

I have tested the patch and can verify the situations in the description all result in ONHOLD.


Thanks,

David


_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Back to top
dimas at dataart.com
Guest





PostPosted: Tue May 26, 2009 9:31 pm    Post subject: [asterisk-dev] [Code Review] hints with 2+ devices that incl

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/#review798
-----------------------------------------------------------



/trunk/main/devicestate.c
<http://reviewboard.digium.com/r/254/#comment1982>

The check in this position means ONHOLD status has a "priority" over BUSY/RING/INUSE. I'm not sure it is good choice - to me at least INUSE/BUSY are more "strong" than INHOLD. Because of that I would put this check last.


- dimas


On 2009-05-26 12:55:56, David Vossel wrote:
Quote:

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/
-----------------------------------------------------------

(Updated 2009-05-26 12:55:56)


Review request for Asterisk Developers.


Summary
-------

This is a community patch I cleaned up and tested. Below is the reporter's, p_lidheimer, description of the issue pulled directly from issue #15057.

----------------------------
If you have a hint with two (or more) devices or a device and a custom devstate that includes ONHOLD, you get unexpected results. Two examples:

exten => 222,hint,SIP/222&SIP/211
exten => 333,hint,Custom:DND333&SIP/333

The following results are inconsistent as they should all return ONHOLD:

ONHOLD & IDLE = IDLE
ONHOLD & UNAVAILABLE => UNAVAILABLE
ONHOLD & UNKNOWN => IDLE
ONHOLD & INVALID => IDLE

Per Russell Bryant's response on the mailing list:

"I would agree that this is a bug. I would expect that combination of
states to result in an OnHold state."


This addresses bug 15057.
https://issues.asterisk.org/view.php?id=15057


Diffs
-----

/trunk/include/asterisk/devicestate.h 196415
/trunk/main/devicestate.c 196415

Diff: http://reviewboard.digium.com/r/254/diff


Testing
-------

I have tested the patch and can verify the situations in the description all result in ONHOLD.


Thanks,

David




_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Back to top
dvossel at digium.com
Guest





PostPosted: Tue May 26, 2009 9:40 pm    Post subject: [asterisk-dev] [Code Review] hints with 2+ devices that incl

Quote:
On 2009-05-26 17:23:44, dimas wrote:
> /trunk/main/devicestate.c, line 799
> <http://reviewboard.digium.com/r/254/diff/1/?file=5429#file5429line799>
>
> The check in this position means ONHOLD status has a "priority" over BUSY/RING/INUSE. I'm not sure it is good choice - to me at least INUSE/BUSY are more "strong" than INHOLD. Because of that I would put this check last.

ahh yes, This was in p_lidheimer's original patch, I accidentally changed it. Thanks!


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/#review798
-----------------------------------------------------------


On 2009-05-26 12:55:56, David Vossel wrote:
Quote:

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/
-----------------------------------------------------------

(Updated 2009-05-26 12:55:56)


Review request for Asterisk Developers.


Summary
-------

This is a community patch I cleaned up and tested. Below is the reporter's, p_lidheimer, description of the issue pulled directly from issue #15057.

----------------------------
If you have a hint with two (or more) devices or a device and a custom devstate that includes ONHOLD, you get unexpected results. Two examples:

exten => 222,hint,SIP/222&SIP/211
exten => 333,hint,Custom:DND333&SIP/333

The following results are inconsistent as they should all return ONHOLD:

ONHOLD & IDLE = IDLE
ONHOLD & UNAVAILABLE => UNAVAILABLE
ONHOLD & UNKNOWN => IDLE
ONHOLD & INVALID => IDLE

Per Russell Bryant's response on the mailing list:

"I would agree that this is a bug. I would expect that combination of
states to result in an OnHold state."


This addresses bug 15057.
https://issues.asterisk.org/view.php?id=15057


Diffs
-----

/trunk/include/asterisk/devicestate.h 196415
/trunk/main/devicestate.c 196415

Diff: http://reviewboard.digium.com/r/254/diff


Testing
-------

I have tested the patch and can verify the situations in the description all result in ONHOLD.


Thanks,

David




_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Back to top
p_lindheimer at yahoo.com
Guest





PostPosted: Wed May 27, 2009 3:20 pm    Post subject: [asterisk-dev] [Code Review] hints with 2+ devices that incl

Thanks for the review and the issue already pointed out. It brought up additional issues so I have attached two new patches for 1.4 and trunk. I made the following comment in the ticket: https://issues.asterisk.org/view.php?id=15057

TICKET COMMENT:

There were some issues pointed out in the review which uncovered some more in further testing, so attaching two new patches that should now exhibit the following behavior:

BUSY & ONHOLD => BUSY
INUSE & ONHOLD => INUSE
ONHOLD & RINGING => RINGINUSE

this opens up a question, should the last return RINGINUSE or is a new state required to return RINGONHOLD? (and if we have ONHOLD&RINGING&INUSE do we return all three?)

My opinion would be RINGINUSE is appropriate...

Quote:


Message: 1
Date: Tue, 26 May 2009 17:55:57 -0000
From: "David Vossel" <[url=/mc/compose?to=dvossel@digium.com]dvossel@digium.com[/url]>
Subject: [asterisk-dev] [Code Review] hints with 2+ devices that
    include    ONHOLD
To: "David Vossel" <[url=/mc/compose?to=dvossel@digium.com]dvossel@digium.com[/url]>,    "Asterisk Developers"
    <[url=/mc/compose?to=asterisk-dev@lists.digium.com]asterisk-dev@lists.digium.com[/url]>
Message-ID: <[url=/mc/compose?to=20090526175557.7403.22626@hotblack.digium.internal]20090526175557.7403.22626@hotblack.digium.internal[/url]>
Content-Type: text/plain; charset="utf-8"


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/
-----------------------------------------------------------

Review request for Asterisk Developers.


Summary
-------

This is a community patch I cleaned up and tested.  Below is the reporter's, p_lidheimer, description of the issue pulled directly from issue #15057.

----------------------------
If you have a hint with two (or more) devices or a device and a custom devstate that includes ONHOLD, you get unexpected results. Two examples:

exten => 222,hint,SIP/222&SIP/211
exten => 333,hint,Custom:DND333&SIP/333

The following results are inconsistent as they should all return ONHOLD:

ONHOLD & IDLE = IDLE
ONHOLD & UNAVAILABLE => UNAVAILABLE
ONHOLD & UNKNOWN => IDLE
ONHOLD & INVALID => IDLE

Per Russell Bryant's response on the mailing list:

"I would agree that this is a bug. I would expect that combination of
states to result in an OnHold state."


This addresses bug 15057.
    https://issues.asterisk.org/view.php?id=15057


Diffs
-----

  /trunk/include/asterisk/devicestate.h 196415
  /trunk/main/devicestate.c 196415

Diff: http://reviewboard.digium.com/r/254/diff


Testing
-------

I have tested the patch and can verify the situations in the description all result in ONHOLD.


Thanks,

David




*****************************

Back to top
dvossel at digium.com
Guest





PostPosted: Wed May 27, 2009 6:04 pm    Post subject: [asterisk-dev] [Code Review] hints with 2+ devices that incl

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/
-----------------------------------------------------------

(Updated 2009-05-27 13:52:53.375055)


Review request for Asterisk Developers.


Changes
-------

update with p_lindheimer's changes.

--------description of update pulled directly from notes on issue #15057
There were some issues pointed out in the review which uncovered some more in further testing, so attaching two new patches that should now exhibit the following behavior:

BUSY & ONHOLD => BUSY
INUSE & ONHOLD => INUSE
ONHOLD & RINGING => RINGINUSE

this opens up a question, should the last return RINGINUSE or is a new state required to return RINGONHOLD? (and if we have ONHOLD&RINGING&INUSE do we return all three?)

My opinion would be RINGINUSE is appropriate...


Summary
-------

This is a community patch I cleaned up and tested. Below is the reporter's, p_lidheimer, description of the issue pulled directly from issue #15057.

----------------------------
If you have a hint with two (or more) devices or a device and a custom devstate that includes ONHOLD, you get unexpected results. Two examples:

exten => 222,hint,SIP/222&SIP/211
exten => 333,hint,Custom:DND333&SIP/333

The following results are inconsistent as they should all return ONHOLD:

ONHOLD & IDLE = IDLE
ONHOLD & UNAVAILABLE => UNAVAILABLE
ONHOLD & UNKNOWN => IDLE
ONHOLD & INVALID => IDLE

Per Russell Bryant's response on the mailing list:

"I would agree that this is a bug. I would expect that combination of
states to result in an OnHold state."


This addresses bug 15057.
https://issues.asterisk.org/view.php?id=15057


Diffs (updated)
-----

/trunk/include/asterisk/devicestate.h 196415
/trunk/main/devicestate.c 196415

Diff: http://reviewboard.digium.com/r/254/diff


Testing
-------

I have tested the patch and can verify the situations in the description all result in ONHOLD.


Thanks,

David


_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Back to top
russell at digium.com
Guest





PostPosted: Thu May 28, 2009 2:01 pm    Post subject: [asterisk-dev] [Code Review] hints with 2+ devices that incl

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/#review803
-----------------------------------------------------------


The changes here look fine.

I think that it would be a great idea to write a test_devstate module that exercises the devstate_aggregate API with a bunch of different combinations of states and verifies that we get the expected results.

- Russell


On 2009-05-27 13:52:53, David Vossel wrote:
Quote:

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/254/
-----------------------------------------------------------

(Updated 2009-05-27 13:52:53)


Review request for Asterisk Developers.


Summary
-------

This is a community patch I cleaned up and tested. Below is the reporter's, p_lidheimer, description of the issue pulled directly from issue #15057.

----------------------------
If you have a hint with two (or more) devices or a device and a custom devstate that includes ONHOLD, you get unexpected results. Two examples:

exten => 222,hint,SIP/222&SIP/211
exten => 333,hint,Custom:DND333&SIP/333

The following results are inconsistent as they should all return ONHOLD:

ONHOLD & IDLE = IDLE
ONHOLD & UNAVAILABLE => UNAVAILABLE
ONHOLD & UNKNOWN => IDLE
ONHOLD & INVALID => IDLE

Per Russell Bryant's response on the mailing list:

"I would agree that this is a bug. I would expect that combination of
states to result in an OnHold state."


This addresses bug 15057.
https://issues.asterisk.org/view.php?id=15057


Diffs
-----

/trunk/include/asterisk/devicestate.h 196415
/trunk/main/devicestate.c 196415

Diff: http://reviewboard.digium.com/r/254/diff


Testing
-------

I have tested the patch and can verify the situations in the description all result in ONHOLD.


Thanks,

David




_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Back to top
Display posts from previous:   
   AsteriskGuru Archives Forum Index -> Asterisk-Dev All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum


Powered by phpBB © 2001, 2005 phpBB Group
contact us at: support@asteriskguru.com - asterisKGuru.com © all rights reserved   |   *asterisk is registered trademark of © Digium™