Skip to content

Conversation

@ndoo
Copy link
Contributor

@ndoo ndoo commented Jul 14, 2025

Independent of existing code, but open to suggestions/feedback.

@ndoo ndoo force-pushed the sbdix-adjust-timeout branch from 371f614 to 0d5d68b Compare July 14, 2025 08:30
@ndoo
Copy link
Contributor Author

ndoo commented Jul 14, 2025

Rebased on main HEAD

@ndoo ndoo marked this pull request as draft July 14, 2025 08:45
@ndoo
Copy link
Contributor Author

ndoo commented Jul 14, 2025

Converting to draft, it only works if called after begin(), so I think this has to be tracked as a variable...

@ndoo ndoo force-pushed the sbdix-adjust-timeout branch 6 times, most recently from b496c43 to d34a52c Compare July 14, 2025 09:43
@ndoo ndoo marked this pull request as ready for review July 14, 2025 09:44
@ndoo
Copy link
Contributor Author

ndoo commented Jul 14, 2025

Ready for review, tried to reduce the impact as much as possible.

Copy link
Collaborator

@PaulZC PaulZC left a comment

Choose a reason for hiding this comment

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

This is looking good Andrew (@ndoo ),

Please have a think about adding the changes suggested below.

I can't see any potential problems. And it maintains backwards compatibility. Thanks!

I don't know what would happen if the user called this while a SBDIX was already in progress? But that's a very fringe case...

Signed-off-by: Andrew Yong <me@ndoo.sg>
@ndoo ndoo force-pushed the sbdix-adjust-timeout branch from d34a52c to 68c112d Compare July 15, 2025 05:26
@ndoo ndoo requested a review from PaulZC July 15, 2025 05:27
@ndoo
Copy link
Contributor Author

ndoo commented Jul 15, 2025

Force pushed with requested changes + minor code style change for the if statement (curly brace on new line)

@ndoo
Copy link
Contributor Author

ndoo commented Jul 15, 2025

I think this is not possible to call during SBDIX with the current library as AFAICT there is no async functionality, so SBDIX is blocking.

@PaulZC
Copy link
Collaborator

PaulZC commented Jul 15, 2025

Thank you for the updates Andrew (@ndoo ),

I will let this sit for a few days, just in case you want to add or change anything. Please give me a nudge if I have not merged it by this time next week.

Best wishes,
Paul

@ndoo
Copy link
Contributor Author

ndoo commented Jul 15, 2025

Thank you for the updates Andrew (@ndoo ),

I will let this sit for a few days, just in case you want to add or change anything. Please give me a nudge if I have not merged it by this time next week.

Sounds good to me. It'll give us some time to let our code cycle through a bunch of Tx cycles too. If I notice any regressions or ill effects I'll likewise communicate here.

Cheers.

@ndoo
Copy link
Contributor Author

ndoo commented Jul 24, 2025

Hi there @PaulZC, the PR has been working smoothly in my testing across ~6 different modems, 30-min SBDIX messages over the past weeks since the PR was raised.

Not sure if you can find more testers, but hopefully there is no regression from this approach.

Short snippet of the way I'm using it:

#define SATELLITE_TIMEOUT_SBDIX 30
satelliteModem.adjustATTimeout(SATELLITE_TIMEOUT_SBDIX + 2);
satelliteModem.adjustSBDSessionTimeout(SATELLITE_TIMEOUT_SBDIX);
satelliteSerial.begin(19200);
int res = satellite.Modem.begin();
// ...

@PaulZC PaulZC changed the base branch from main to release_candidate July 30, 2025 10:41
@PaulZC
Copy link
Collaborator

PaulZC commented Jul 30, 2025

Thank you Andrew (@ndoo ),

Merging...

@PaulZC PaulZC merged commit 0cb6144 into sparkfun:release_candidate Jul 30, 2025
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.

2 participants