Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: call - add call on-hold/resume test #2775

Merged
merged 11 commits into from
Oct 30, 2023

Conversation

cspiel1
Copy link
Collaborator

@cspiel1 cspiel1 commented Oct 23, 2023

relates to: baresip/re#990
try to reproduce: #2772

  • adds call on hold and resume test
  • adds function to wait for SIP ACK

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 23, 2023

I added a function to re: baresip/re#990

Does someone have a better idea?

@cspiel1 cspiel1 marked this pull request as ready for review October 23, 2023 09:39
@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 23, 2023

The last commit replaces ACK check from UA a to b. Because b sends the SIP Reply also b has to wait for the ACK.

Now the ACK check fails because it was not received during re_main. A reason could be that the loop was canceled to early.

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 23, 2023

For a correct cancel rule that waits for an ACK maybe we need an event/callback function. Maybe the new getter function is not enough. I have to think if we can avoid this.

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 23, 2023

@sreimers do you have a better idea to implement a wait for ACK?

  • At least we should avoid the timeout warnings somehow.
  • The last commit also adds an ugly sleep of 1ms.

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 23, 2023

re-based to main HEAD after merge of 33047e6

@sreimers
Copy link
Member

sreimers commented Oct 23, 2023

@sreimers do you have a better idea to implement a wait for ACK?

  • At least we should avoid the timeout warnings somehow.
  • The last commit also adds an ugly sleep of 1ms.

Not sure if I understand all details of the problem, is it maybe possible to add/register or reuse any callback instead of the active wait loop?

edit: Ah I see, you suggested something similar already.

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 23, 2023

Not sure if I understand all details of the problem, is it maybe possible to add/register or reuse any callback instead of the active wait loop?

edit: Ah I see, you suggested something similar already.

Yes, but as we know another callback increases the complexity. Currently there is no callback that can be used. Except if we put an SDP into the ACK. This would test something different, a special case where the offer is contained in the SIP reply and the answer in the ACK, if I understood ack_handler() in listen.c correctly.

@sreimers
Copy link
Member

Maybe a active tmr callback works better in this case, it's not perfect but should prevent timeout warnings.

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 23, 2023

I tried to add an sipsess_ack_h callback. This is too much only for unit tests. BTW: Found that we have a redundant use of call->arg. It can be overwritten by ABI function call_set_handlers().

The timer is a good idea for checking the ACK.

@cspiel1 cspiel1 force-pushed the test_call_hold_resume branch 2 times, most recently from 33eff9e to bd630df Compare October 24, 2023 12:18
@alfredh
Copy link
Collaborator

alfredh commented Oct 25, 2023

Thanks for writing this test. This looks like a good addition :)

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 25, 2023

Your welcome! In following PRs we could add the wait for ACK condition to more tests. Maybe other re-INVITE tests.

@alfredh
Copy link
Collaborator

alfredh commented Oct 26, 2023

please resolve the conflicts ..

Wait for ACK should not check n_incoming nor n_progress
@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 30, 2023

This is ready to merge from my side. Any more suggestions?

@alfredh alfredh merged commit 1747521 into baresip:main Oct 30, 2023
17 checks passed
@cspiel1 cspiel1 deleted the test_call_hold_resume branch October 30, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants