GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Various changes from IAXmodem's spandsp made in the last few years. by redder86 · Pull Request #42 · freeswitch/spandsp

Conversation

@redder86

@redder86 redder86 commented Jun 4, 2022

Copy link
Copy Markdown

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
redder86 added 8 commits June 4, 2022 07:02
@redder86
Accept poor fast-train training errors. This affords the DTE the oppo…
9a5f54c
…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.
@redder86
ITU-T V.250 6.3.1 defines the ATD command. It states: 'Any characters…
cae078c
… 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.
@redder86
Prevent multiple CONNECT messages after +FRH=3 which could have been …
b4d2874
…occurring when the DTE was slow enough that spandsp would buffer messages.
@redder86
Send NO CARRIER message if hangup occurs during data transmit
2949155
@redder86
Send NO CARRIER message if hangup occurs during data transmit (2)
3955808
@redder86
Correctly address the issue attempted to be fixed by the previous com…
f4790de
…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.
@redder86
This CONNECT message is superfluous. If there is some delay in the DT…
cf204c8
…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.
@redder86
These changes are a contribution from Robert Boisvert and the Alpine …
a701c59
…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.
@andywolk
andywolk requested a review from coppice-git June 5, 2022 09:47
@coppice-git

coppice-git commented Jun 5, 2022

Copy link
Copy Markdown
Collaborator

I'm still reviewing the first of the 8 changes.
Changes 2 to 7 are good, and should be merged.
Change 8 should not be merged. Other changes to the code have surpassed it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

redder86 added 8 commits June 8, 2022 16:21
@redder86
iaf is treated as a bitmap everywhere else in the code, not a boolean…
f5943c7
…, and its function as a bitmap is necessary, rather than as a boolean.
@redder86
The 3rd octet of the FIF does, indeed, indicate the length of the int…
461bf79
…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.
@redder86
Revert "These changes are a contribution from Robert Boisvert and the…
056a9e6
… 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.
@redder86
Add t30_set_retransmit_capable API function to allow access to retran…
c410e11
…smit_capable.
@redder86
If retransmit_capable is not enabled then spandsp's behavior followin…
becb49f
…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.
@redder86
correction to the previous commit
2daa783
@redder86
If a terminal problem occurs as a receiver in Phase B such that we'll…
0635d6b
… 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.
@redder86
This adds HylaFAX+ SSL Fax client support.
472ca5e
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.
@freeswitch-ci-system

freeswitch-ci-system Bot commented Jul 27, 2022

Copy link
Copy Markdown

Scan-build compilation failed: https://public-artifacts.signalwire.cloud/drone/freeswitch/spandsp/30/scan-build-result.txt

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coppice-git coppice-git Awaiting requested review from coppice-git

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@redder86 @coppice-git
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.