-
Notifications
You must be signed in to change notification settings - Fork 395
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
Added play_dtmf stuff #141
Conversation
If you guys want this cleaned up in any way before landed, feel free to ask. |
Sounds awesome. Although I tested none of it ;) It does need some minor code style cleanup here and there. And it needs documentation. Unfortunately there is no documentation in this repository. (There is https://github.com/SIPp/sipp-docs -- but it hasn't been given much recent love.) Do you have an example scenario that works with this? If that were added into the binary, it would be sort of self-documenting. |
Sounds good to me. I'll be a little while, but yeah, I can tackle that. We're interested in using this internally since we've been doing what this patch supports with pcap recording. |
I wonder if it makes sense to setup a gh-page for sipp then, btw. So we can host up-to-date docs here. |
Worth noting that I've got http://sipp.readthedocs.org/en/latest/ (built off https://github.com/SIPp/sipp/tree/documentation) - I think moving to reStructuredText and readthedocs.org is better than the crusty Apache Forrest stuff at https://github.com/SIPp/sipp-docs, but the migration is a bit of a pain so I haven't got very far yet. |
@rkday Oh damn, I didn't see you started doing this. I would have asked to help contribute instead of going off on my own with this. I took a different approach with gh-pages/jekyll (but also didn't get very far). You can see what I have thus far at http://vodik.github.io/sipp (haven't bothered with a theme yet). As far as format goes, I went with markdown but I'm familiar with rst as well. |
231edd4
to
341fb97
Compare
Hi, Do you have any plans to merge this commit in the near future? We use SIPp for testing in our organisation and not having DTMF is a big impediment. Thanks |
Hi @a-tinsmith, does this branch work for you in its current form? If so, I can probably get it cleaned up and merged quickly. |
(At @vodik and others: I think I'll branch current master to a 3.5.x branch so we can get this new feature into master (3.6.x) only, and leave 3.5.x for trivial fixes and features only.) |
Hi @wdoekes, Thank you for the prompt reply. Yes, it works for me. The merge of this feature into the main codeline will be much appreciated. |
Hey, yeah, I'm still around. I've give it a look over. I really need to resume my work here anyways. |
@vodik: I created a branch/3.5 for the purpose of bug fixes to currently released 3.5.x versions. This one can go into master as a new feature, as far as I'm concerned. (Fixes to both will have to be cherry-picked from master.) |
I'm going to clean it up a bit and merge it onto master then, and then I'll start using it. But looks like its an incomplete implementation. A, B, C and D tones are missing. |
And little endian only as well. |
Signed-off-by: Simon Gomizelj <[email protected]>
Okay, looks like I either broke something or this didn't quite work. I'm not seeing RTP events in Wireshark if I use this... I'll see what I can figure out this weekend. |
Okay, so this patch works correctly, the problem is it hardcodes the rtp payload type to 96 (the first dynamic payload). The scenarios I was testing this patch inside had set port 101 in the SDP. So that's something that still needs to be addressed, but I guess shouldn't prevent the code from going in if people are finding it useful now. Enjoy @a-tinsmith! |
Thanks for picking this up @vodik! |
Thanks a lot! Much appreciated. :) |
This is the patch from #82 but rebased onto current upstream master.