Various changes from IAXmodem's spandsp made in the last few years. #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rtunity to make some use of whatever valid Phase C data can be decoded. Some data is better than no data even if that data is messy.
… appearing in the dial string that the DCE does not recognize as a valid part of the call addressing information or as a valid modifier shall be ignored. This permits characters such as parentheses and hyphens to be included that are typically used in formatting of telephone numbers.' The permissible dialing digits are defined in the following section: 6.3.1.1. The '+' character is permissible, as with *, #, A, B, C, and D.
…occurring when the DTE was slow enough that spandsp would buffer messages.
…mit. If a hangup occurred after the DTE sent DLE+ETX (following AT+FTM=n + CONNECT, for example) but before all of the data had been transmitted and the OK result given, then that would trigger a problem. This was most common in sending TCF because it was so short. For some reason unknown to me t31.c had the modem being put in command mode before the OK message... this resulted in the lack of the NO CARRIER message on hangup. Removing the problematic command mode instigator seems to be harmless as it seems to have been superfluous - it was being put in command mode again with the OK.
…E providing data to the DCE after +FTH=3 then this was causing CONNECT to be repeated needlessly and confusingly. The CONNECT message happening in hdlc_tx_underflow() is adequate.
…Linux team to address an issue that was encountered while testing with valgrind. The reported problem was that of an 'uninitialized value', and Robert Boisvert's statement was that the changes came from 'upstream'... although clearly he was not referring to upstream spandsp proper. By 'upstream' he may simply have been reporting what was told to him by others, and 'upstream' may actually mean some other fork of spandsp somewhere. That said, the claim was that this change resolved the issue as reported by valgrind. I'm not sure why the code needed to be so refactored as it was in order to resolve an uninitialized value, and as many times as I've examined this I've never really understood the uninitialized value issue here. However, admittedly the changed code is easier to follow, and as at_cmd_plus_VSID() was the only function to utilize parse_string_out(), I ultimately felt good about adopting it.
|
I'm still reviewing the first of the 8 changes. |
…, and its function as a bitmap is necessary, rather than as a boolean.
…ernet address/url, however, because pkt includes the FCF byte as the first octet, we find the internet address length in the 4th octet of pkt and not the 3rd.
… Alpine Linux team to address an issue that was encountered while testing with valgrind. The reported problem was that of an 'uninitialized value', and Robert Boisvert's statement was that the changes came from 'upstream'... although clearly he was not referring to upstream spandsp proper. By 'upstream' he may simply have been reporting what was told to him by others, and 'upstream' may actually mean some other fork of spandsp somewhere. That said, the claim was that this change resolved the issue as reported by valgrind. I'm not sure why the code needed to be so refactored as it was in order to resolve an uninitialized value, and as many times as I've examined this I've never really understood the uninitialized value issue here. However, admittedly the changed code is easier to follow, and as at_cmd_plus_VSID() was the only function to utilize parse_string_out(), I ultimately felt good about adopting it." This reverts commit a701c59. Discussion with Steve Underwood indicates that whatever the uninitialized value issue was here, it has been remedied previously, and the code refactoring is undesireable.
…g receipt of RTN is to just move on to the next page. While this is not uncommon behavior for various fax senders (and admittedly even HylaFAX has this type of behavior as a non-default configurable option), I believe that it is incorrect behavior. If the system cannot retransmit the page following RTN (or also PIN) then the sender should send DCN and consider the transmission attempt as unsuccessful. As the current spandsp behavior without retransmit_capable being enabled essentially treats RTN identical to MCF this becomes confusing to users who get an indication of a successful transmission but where the pages transferred do not equal the total number of pages. This is especially confusing when the total number fo pages is only one... because the receiver very well may not have received/printed any page at all and be no wiser at all to the failed fax receipt - and yet spandsp on the sender's end is identifying the fax as successful (and so the sender may not know that there was an issue unless they are careful to compare the pages transferred to the total number of pages). This change here causes spandsp to identify the fax as unsuccessful if retransmit_capable is not enabled and RTN was received on the only page of a fax.
… be sending DCN we must wait until after the sender transmits TCF, otherwise the sender will not likely hear the DCN signal. This corrects that problem.
Some of the code feels a bit like it's being shoehorned-in. I'm sure that Steve may prefer some of this to be written differently. However, it works properly, and the code seems legible; that's what mattered to me. As support for SSL Fax client was my primary objective and immediate interest this does not add SSL Fax server support or support for SSL Fax proxy servers. I may likely implement those at some later time if nobody beats me to it. Because spandsp is driven by event-based triggers (primarily time intervals) it naturally involves some very brief delays between SSL Fax operations which are not there in HylaFAX+. That said, these delays are likely irrelevant and probably will go unnoticed. These SSL Fax operations have been tested against both HylaFAX+ and Mainpine IQFSP in both ECM and non-ECM sessions. I believe it to be reliable.
|
Scan-build compilation failed: https://public-artifacts.signalwire.cloud/drone/freeswitch/spandsp/30/scan-build-result.txt |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.