Skip to content

WIP: Implement a Tokio Codec for SIP #38

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

csicar
Copy link

@csicar csicar commented Apr 3, 2022

not sure if you would want add a Tokio Codec rsip

In case you do, I'd clean this up, add more tests and an example.

Also: It seems that you don't yet use a logging system (which could be useful for clients). If you want, I'd add that in as well to replace the println macro calls

@vasilakisfil
Copy link
Collaborator

vasilakisfil commented Apr 4, 2022

Hi @csicar. Implementing Tokio codec is on my TODO list :) basically you need that if you are going to use a non-UDP connection. So your code will be quite useful for me, thanks for that. Regarding logging, I have tried to avoid that and implement anything that related to logging to errors instead and let the user of rsip decide what to do with that. I haven't looked much to Tokio codec, but if logging is something that makes sense to include there, I won't hesitate to do that, after all the whole thing will be very probably under a feature flag.

At the moment and for the next 1-2 months, I am focusing on some things on dialogs/TU in the viska sip framework that uses rsip, but will come back here very soon since there I have only implemented UDP transport and TCP transport is my very next thing to do.

Leaving this open until I address it properly.

@csicar
Copy link
Author

csicar commented Apr 6, 2022

That sounds great. In that case I'll clean the PR up, so that it could be merged.

The codec is already testes with Twinkle.

In reads to logging: I think logging the raw sip requests could come in handy for library users. I also agree, that it would need to be behind a feature flag

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.

2 participants