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
res_pjsip_device_features: Add forwarding and DND synchronization. #128
Conversation
This adds support for synchronization of device features, specifically call forwarding, ring control, and Do Not Disturb, according to the Broadworks specifications for device feature key synchronization. This allows Asterisk to support IP phones that support device feature key sync the same way that a Broadworks server would. The implementation of this feature is designed to be flexible enough to allow administrators to configure their own process for handling feature toggle and set requests. To allow this, AMI events and actions are present to interface with an administrator's business logic. Resolves: asterisk#127 UpgradeNote: Device feature key sync for call forwarding and Do Not Disturb has been added to PJSIP.
3fdb875
to
0bf24f2
Compare
#include "asterisk/xml.h" | ||
#include "asterisk/taskprocessor.h" | ||
#include "asterisk/app.h" | ||
#include "asterisk/astdb.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use old AstDB instead of Sorcery?
I think all new modules especially PJSIP modules should use Sorcery.
In case of Sorcery the user can choose where to store synchronized data: astdb, memory, realtime DB, memory_cache.
All PJSIP objects are stored using Sorcery, so these PJSIP device features should also be stored using Sorcery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AstDB is mainly used as a cache between restarts, the same way PJSIP uses it for subscriptions, for example. It's really the admin's responsibility to store them in the desired place anyways as part of the approval process, which is outside of the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PJSIP subscription_persistence uses Sorcery and astdb storage by default.
ast_sorcery_apply_default(sorcery, "subscription_persistence", "astdb", "subscription_persistence");
But the user can choose another storage.
As I know nor PJSIP core neither PJSIP modules use astdb directly.
/* Whether we should auto-approve device feature changes. | ||
* If disabled, system will use the AMI event to update feature data and write PJSIP_DEVICE_FEATURES to trigger a NOTIFY. | ||
* If enabled, we will "auto approve": internal cache will automatically update per the request and a NOTIFY will be sent. */ | ||
static int auto_approve = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's constant. I could not find the code which can change this.
I think this should be PJSIP Endpoint parameter, it means some endpoints are allowed to change it automatically, some endpoints not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This absolutely needs to be configurable per-endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to confirm, it's fine to do this in pjsip.conf, as an endpoint property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoint property. For example
ast_sorcery_object_field_register(sip_sorcery, "endpoint", "allow_device_features_sync", "no", OPT_BOOL_T, 1, FLDSET(struct ast_sip_endpoint, allow_device_features_sync));
{ | ||
int i; | ||
|
||
/* XXX This is kind of a kludge. In order to send updates for more than one feature, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do this "kind of a kludge" and send many NOTIFYs instead of just one with multipart?
Please look at pjsip_multipart_* functions.
https://www.pjsip.org/pjsip/docs/html/group__PJSIP__MULTIPART.htm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://www.cisco.com/c/dam/en/us/td/docs/voice_ip_comm/broadworks/FD/AS/DeviceFeatureKeySynchronizationFD.pdf
2.2 Call Flows
2.2.1 Initial SUBSCRIBE and NOTIFY
"When a supported phone powers up, it sends a SUBSCRIBE request to the Application
Server to create a subscription for the as-feature-event package and to get the full feature
status. "
"The Application Server sends a NOTIFY request to the phone
with a body that contains the complete feature status, and the phone sends a 200
response. The body is a MIME multipart body, containing an XML document for each
feature assigned to the subscriber".
*/ | ||
struct ast_sip_device_feature_sync_data { | ||
/* Device ID */ | ||
char deviceid[64]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this 64 value come from? What does it represent in relation to the rest of the PJSIP work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would hold the device name (channel name before the -).
Where is the #define for the max device name length? I'd been looking for that for a long time and could never find one. I know you and Sean were discussing the one for channel name length, but that would be longer than necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "struct ast_sip_device_feature_sync_data" should be the sorcery object. For example
struct ast_sip_device_feature_sync_data {
/*! Sorcery object details, the id is the ENDPOINT name */
SORCERY_OBJECT(details);
AST_DECLARE_STRING_FIELDS(
AST_STRING_FIELD(fwd_exten_always);
AST_STRING_FIELD(fwd_exten_busy);
AST_STRING_FIELD(fwd_exten_noanswer);
);
unsigned int dnd;
unsigned int ring_count;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, forget deviceid
AST_STRING_FIELD(deviceid);
* So we use the core XML routines. */ | ||
root = ast_xml_get_root(xmldoc); | ||
if (!root) { | ||
ast_log(LOG_WARNING, "No root?\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meaningless to a user
/* | ||
* Asterisk -- An open source telephony toolkit. | ||
* | ||
* Copyright (C) 2022, Naveen Albert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2023
<para>Name of the feature.</para> | ||
<para>Must be one of:</para> | ||
<enumlist> | ||
<enum name="donotdisturb"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the words should be separated by _ ala do_not_disturb
/* Whether we should auto-approve device feature changes. | ||
* If disabled, system will use the AMI event to update feature data and write PJSIP_DEVICE_FEATURES to trigger a NOTIFY. | ||
* If enabled, we will "auto approve": internal cache will automatically update per the request and a NOTIFY will be sent. */ | ||
static int auto_approve = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This absolutely needs to be configurable per-endpoint.
ast_debug(1, "ForwardingEvent update required (%s)\n", fwd_str); | ||
doc = ast_xml_new(); | ||
if (!doc) { | ||
ast_log(LOG_ERROR, "Could not create new XML document\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meaningless to a user
/* The Broadworks spec says device is mandatory, but the actual value is used by neither server nor client. So if we don't have one, make something up. */ | ||
ast_xml_set_text(tmpnode, S_OR(sync_data->deviceid, "123456")); | ||
if (!sync_data->deviceid[0]) { | ||
ast_debug(2, "This was the first NOTIFY with body data for this endpoint\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs more information (such as endpoint)
ast_xml_doc_dump_memory(doc, &buf, &len); | ||
ast_xml_close(doc); | ||
if (len <= 0) { | ||
ast_log(LOG_WARNING, "XML document has length %d?\n", len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meaningless to a user
return 0; | ||
cleanup: | ||
ast_xml_close(doc); | ||
ast_log(LOG_ERROR, "Could not create new XML root node\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meaningless to a user
int updates_made = 0; | ||
|
||
/* This callback is called for *all* NOTIFYs, so we should only add XML to the body if actually necessary. */ | ||
ast_debug(2, "Generating body content for %s/%s\n", FEATURE_TYPE, FEATURE_SUBTYPE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs more information such as the endpoint, and so on for other log messages, and meaningless messages for users too
I also agree with Alexei's comments |
Could you please explain "administrator's business logic."? As I understand with this module the SIP phones can synchronize with the Application Server the status of the features: DND, CFA, CFB, and CFNA, i.e. if a user changes the status of one of these features via web portal the Application Server notifies the phone about the status change. Conversely, if the user changes the feature status via a button or a menu on the phone, the phone notifies the Application Server of the status change. I see the business logic as: |
Business logic here meaning that Asterisk itself may neither be capable of approving requests itself nor be the source of the truth for this information. The TL;DR is that this module does not fully control any of these features. For example, I may have an IP phone and an analog DAHDI line that share the same feature state, and there may be dialing codes and other mechanisms to control the features. How they do that is totally implementation dependent. The PJSIP module here is only to handle the SIP side of synchronized feature state. It is not really intended to handle the feature implementations themselves. In theory it could (that's basically what auto approve is), but that is simplistic and overly limiting, and so control is explicitly configurable by the administrator if desired. As an example, I'll describe my implementation. The PJSIP module here is only part of how the system works. I can activate Do Not Disturb by dialing *77, or toggling the key on a Polycom phone, or a Recent Change message or other out of band mechanism that updates the switch's internal database (which is outside of Asterisk). All of these processes need to agree upon a common way of communicating. This is not dictated (and should not be dictated) by the PJSIP module, In my case, my feature data is permanently stored in the switch database and certain small things in AstDB (like whether a feature is active or not), but the actual live status lives in memory in a proprietary Asterisk module (it's literally a single bit whether DND is active for a particular "line"... line being a logical construct in my system that Asterisk itself doesn't know about). I need this to be updated whenever any of the above mechanisms update the feature status. Otherwise, the PJSIP module can "think" the feature is toggled, and it internally might think it is, but it's wrong because this module doesn't actually know or control the feature status directly. When a call goes to a line, it will check the Do Not Disturb bit in memory, it doesn't care or know about the PJSIP module at all. That's business logic because everyone might implement this differently. Similar things apply to all the other settings, too. And in the reverse direction, if I dial *87 on a phone, PJSIP needs to send a NOTIFY to the phone that the feature is turned off. Of course, it won't know my business logic, so I'll need to tell it explicitly about that. That's why there's an interface to expose activation/deactivation requests and set the status for the device: how it interacts with the rest of the system will vary greatly. |
Also big question about multi subscriptions/registrations. |
Multiple contacts on the same AOR? That should already work by default I believe. And even if they are separate AORs, it's easy to achieve in business logic. You can just look up all the devices that should share a common DND state and then send them all updates. But again, that's business logic, not built in. |
Please read "2.1.3 Assigned or Unassigned Features".
The Sorcery data can be modified by any other mechanisms independent from the Asterisk.
The PJSIP module shares this data/information with the Application Server, so both the Asterisk and the Application Server can modified this data and notify about changes.
Following you example. |
Not necessarily. Yes if it's purely binary (on/off), but there may be other factors that influence it which change during runtime. In the simplest auto approve case though, configurable per endpoint (not currently, but it will be), that's how it would work though.
If there's a sorcery backend that supports it. I'm not using anything that would be exposed in a sorcery backend currently, so that doesn't help me. The manual way already requires something that needs to be set regardless either way (right now, it's a dialplan function).
The key point here is that I may not want Asterisk to be allowed to change the state. Changes should succeed only if I tell them to. This is certainly a more complicated use case, and for the auto approve case is irrelevant. But I think in the case that admins opt to not auto approve, they probably have complicated requirements anyways and it's best to be more flexible than less.
What I do currently is change the bit in memory and then call the dialplan function to tell Asterisk of the new state, which then triggers the NOTIFY to go out.
No, because maybe I deny changing the setting. In that case, the change should not have any effect and Sorcery data should not change, because it would be wrong.
Not in my case, the database has a long term store of feature assignments but it's not used by Asterisk directly at all. In my case, the information accessed in real time is generally in memory. Unless a Sorcery frontend /driver were built for that, which I have no intention of doing, Sorcery can't work with that at all, to my knowledge (though not having used Sorcery before, I'm not 100% positive). I think we're mostly on the same page here... in the auto-approve mode, Asterisk is the application server (kind of... it doesn't actually implement the feature, but the feature status could be readily checked somehow). When disabled though, all the module should do is notify the application server about something a device has requested, and communicate to a device a feature setting that the application server has told it is the current setting. All the application logic would sit outside of Asterisk itself, so people can implement it however they like. |
There is Sorcery In-Memory storage module. |
Good to know, but not really relevant here. That's just a backend that stores settings in memory, it's not how I store settings in memory. Fundamentally there is no getting around in a custom environment (no auto approve), the application server will need to tell Asterisk/PJSIP of the new state to send it the current module. PJSIP cannot be the source of truth, because it isn't the center of the feature, it's just hooking into it. I might run a system without PJSIP at all, and it makes no sense to be storing my feature config in a way that is dictated by PJSIP, Sorcery or otherwise. I am not changing my feature implementation or where that data is stored, full stop. The reasonable thing to do is add hooks for getting events and notifying PJSIP, and nothing else needs to be changed. Going back to the original point here, I can look into changing the cache from AstDB to Sorcery, since, at least with auto approve enabled, that might matter somewhat. Aside from that, Sorcery doesn't provide any major benefits I can see here, since it isn't the source of truth, in which case the best option is simply the one that is convenient for the programmer, since end users aren't interacting with the cache directly. |
7c2f720
to
5b7dd28
Compare
1b894e6
to
32fd0fb
Compare
b15287c
to
1862a36
Compare
This PR has been marked stale because it has been in "Changes Requested" or "submitter-action-required" state for 28 days or more. Please make the requested changes within 14 days or the PR will be closed. |
This PR has been closed because it has been in "Changes Requested" or "submitter-action-required" state for more than 42 days. |
This adds support for synchronization of device features, specifically call forwarding, ring control, and Do Not Disturb, according to the Broadworks specifications for device feature key synchronization. This allows Asterisk to support IP phones that support device feature key sync the same way that a Broadworks server would.
The implementation of this feature is designed to be flexible enough to allow administrators to configure their own process for handling feature toggle and set requests. To allow this, AMI events and actions are present to interface with an administrator's business logic.
Resolves: #127
UpgradeNote: Device feature key sync for call forwarding and Do Not Disturb has been added to PJSIP.