Skip to content
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

_processUrcPacket manipulate string in function scope #131

Conversation

chinglee-iot
Copy link
Member

@chinglee-iot chinglee-iot commented Feb 13, 2023

This PR is related issue #130.

  • Remove the urcParseToken function. Manipulate allocated string in function scope in _processUrcPacket only.
  • Use different variable name for different URC pattern to help reader understanding the handling flow.
    • URC with prefix : _atParseGetHandler( pContext, pTokenPtr, pSavePtr );
      pInputLine will be splited into the following format
      pIntputLine = "+" pTokenPtr + " : " + pSavePtr
      pTokenPtr is used to search URC handler with prefix and pSavePtr is the input parameter to the URC handler.
    • URC without prefix : _atParseGetHandler( pContext, pInputLine, pInputLine );
      pInputLine will be used to search URC handler and pInputLine will also be used as input parameter to the URC handler.
    • URC handler not found : _Cellular_GenericCallback( pContext, pInputLine );
      CellularUrcGenericCallback_t will be called. pInputLine is the input parameter to the function.

* Manipulate allocated string in function scope
Move the log line after Cellular_ATStrDup

Co-authored-by: ActoryOu <[email protected]>
ActoryOu
ActoryOu previously approved these changes Feb 13, 2023
ActoryOu
ActoryOu previously approved these changes Feb 13, 2023
* the leading char and split the string into the following substrings :
* pInputLine = "+" pTokenPtr + ":" + pSavePtr. */
pSavePtr = pInputLine + 1;
pTokenPtr = strtok_r( pSavePtr, ":", &pSavePtr );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still retains the issue described in #130

Suggesting the following implementation

        /* Move pTokenPtr to URC name start */
        pSavePtr = pInputLine + 1
        pTokenPtr = pSavePtr;

        /* Scan for : or string end */
        while ( *pSavePtr != ':' && *pSavePtr != 0 )
            pSavePtr++;

        /* Null terminate pTokenPtr and move pSavePtr to URC content start */
        if ( *pSavePtr != 0 )
            *( pSavePtr++ ) = 0;

It is not a direct equivalent (strtok skips multiple consecutive delimiters), but this isn't relevant for our use case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback.
I probably misunderstand your original suggestion.
Are you suggesting that not to use strtok_r and manipulate the string in this function directly?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the reason being that AFAIK the typical standard library implementations (or at least their man pages) do not guarantee what savePtr (usually named lasts) should be, other than vaguely describing that it stores the required state. Some man pages describe it to be internal to strtok_r.

However, I have to admit that I did not check if contemporary implementations actually differ.

Either case it is an opaque piece of code that I viewed often as I had issues with handling URCs at the time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback.
We will look into your suggestion.
If it is feasible to us, we will open another PR to update the implementation.

@@ -183,28 +126,72 @@ static CellularPktStatus_t _processUrcPacket( CellularContext_t * pContext,
const char * pBuf )
{
CellularPktStatus_t pktStatus = CELLULAR_PKT_STATUS_OK;
bool inputWithPrefix = false;
char * pInputLine = NULL;
char * pSavePtr = NULL, * pTokenPtr = NULL;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggesting better names,

  • pSavePtr could be pContent
  • pTokenPtr could be pToken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants