Added Channel Variables and AMI Event for Respoke Session

[gcareri]

Added two new features:

1. Set of Asterisk Channel Variables related to the Respoke Session
2. AMI (Asterisk Manager Interface) new Event 'respoke_session' in order to show the Respoke Session information.

The purpose of such features is to make Respoke information available both on the asterisk channel and on the asterisk manager interface.

[respoke-cla]

Thank you for your Pull Request.

It looks like @gcareri hasn't signed our Contributor License Agreement, yet.

Thanks for your understanding,
Your friendly neighborhood Respoke CLAbot

[tiandavis]

@gcareri Hi Giuseppe - You've probably seem something similar for Asterisk contributions. Here's the Contributor License Agreement (CLA) for chan_respoke:
https://cla.respoke.io/chan_respoke

[tiandavis]

@gcareri Hi Giuseppe - I know the README isn't necessarily part of the code updates. But, could you update the README with how to use these two features?

This will help future chan_respoke developers get started using the new feature independent of where they are on the asterisk learning curve. Grazie Giuseppe!

[tiandavis]

This is really great work @gcareri. I ❤️ the README updates for both the channel variable and AMI events.

👍

[tiandavis]

[clabot:check]

[respoke-cla]

It looks like @gcareri just signed our Contributor License Agreement. 👍

Always at your service,

Your friendly neighborhood Respoke CLAbot

[tiandavis]

@gcareri You're CLA is approved. 😃

[danjenkins]

your AMI events should be PascalCase not snake_case to follow Asterisk convention

[gcareri]

We have modified the AMI event name according to your suggestion (RespokeSession).

Thank you for your suggestion.

[matt-jordan]

For compatibility purposes, I'd CamelCase the keys in the RespokeSession event. That fits the general nomenclature of the other standard AMI events.

For example:

Local: your_respoke_endpoint
LocalType: web

etc.

[creslin287]

This might be a silly question about intent, and perhaps this is where my dial plan weakness shows through. Is there a reason you can't use the CHANNEL([local, remote, ...]) to access the same information as those new channel variables - where most of those pieces of session data (local, remote, local_type, remote_type, and a few others) are already exposed? A couple of your exposed variables don't exist right now, but we could expose those through the CHANNEL() function as well.

The manager event is not provided at this time, so I see that being unique and beneficial.

Looking through your commits it looks like there's been a bit of back and forth on this.

For more information about channel variable reading and writing (on the backend) see func_read_channel() and func_write_channel() in channels/chan_respoke.c

The dial plan interface can be used as follows (or in other ways as well, like a variable):
exten => s,1,NoOp(${CHANNEL(remote)}) ; prints the value of remote to the console
exten => s,1,Set(CHANNEL(local)=something) ; sets the value of the local channel variable to something

[gcareri]

That could certainly be an interesting feature.
However it requires some more efforts and the current Respoke session information converted in to the Uppercase notation for the Asterisk Channel variables solves the most part of our needs.
Please feel free to help in enhancing and modify it according to more specific Asterisk conventions.

[tiandavis]

Forgive me for not picking up on this sooner @gcareri. The issue is the following updates to res/res_respoke_session.c may duplicate existing code:

pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_LOCAL", session->local);
pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_LOCAL_TYPE", session->local_type);
pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_LOCAL_CONNECTION", session->local_connection);
pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_REMOTE", session->remote);
pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_REMOTE_TYPE", session->remote_type);
pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_REMOTE_CONNECTION", session->remote_connection);
pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_REMOTE_APPID", session->remote_appid);
pbx_builtin_setvar_helper(session->channel, "RESPOKE_SESSION_ID", session->session_id);

See, channels/chan_respoke.c and the func_read_channel and func_write_channel functions.

These functions allow you to use the CHANNEL variable to do the following in your dialplan:

exten => s,1,NoOp(${CHANNEL(remote)}) ; prints the value of remote to the console
exten => s,1,Set(CHANNEL(local)=something) ; sets the value of the local channel variable to something

But, currently, they only implement local, local_type, remote, remote_type and remote_appid. In addition to those, they would need to implement local_connection, remote_connection and session_id to be equivalent to your solution here.

I think the final ask is to move this functionality from res/res_respoke_session.c to the func_read_channel and func_write_channel functions in channels/chan_respoke.c. Something like this for local_connection, remote_connection and session_id:

static int func_read_channel(struct ast_channel *chan, const char *function,
                 char *data, char *buf, size_t len)
{
    struct respoke_session *session = ast_channel_tech_pvt(chan);

    . . .

    if (!strcmp(data, "local_connection")) {
        ast_copy_string(buf, ast_sorcery_object_get_id(session->local_connection), len);
    } 

    . . .

    return 0;
}

static int func_write_channel(struct ast_channel *chan, const char *function,
                  char *data, const char *value)
{
    struct respoke_session *session = ast_channel_tech_pvt(chan);

    . . .

    ast_channel_lock(chan);

    if (!strcmp(data, "local_connection")) {
        ast_string_field_set(session, local_connection, value);
    } 

    . . .

    ast_channel_unlock(chan);

    return 0;
}

This has the benefit of not duplicating existing code. I believe ${CHANNEL(remote)} reads a little better than RESPOKE_SESSION_REMOTE as well. Could you make those final changes? Then I believe we are good to merge the PR.

Everything else looks great btw! Thanks Giuseppe!

[chadxz]

Closing due to inactivity. Feel free to re-open if you want to address the feedback given and want to see this merged in.