sat: timeout: Add timeouts for sat transmission#230
Conversation
Prevent a transmission to possibly lock indefinitely seting a timeout for used sempahores. Signed-off-by: Flavio Ceolin <flavio@hubble.com>
Prevent a transmission to possibly lock indefinitely seting a timeout for used sempahores. Signed-off-by: Flavio Ceolin <flavio@hubble.com>
Prevent a transmission to possibly lock indefinitely seting a timeout for used sempahores. Signed-off-by: Flavio Ceolin <flavio@hubble.com>
Prevent a transmission to possibly lock indefinitely seting a timeout for used sempahores. Signed-off-by: Flavio Ceolin <flavio@hubble.com>
Prevent a transmission to possibly lock indefinitely seting a timeout for used sempahores. Signed-off-by: Flavio Ceolin <flavio@hubble.com>
Prevent a transmission to possibly lock indefinitely seting a timeout for used sempahores. Signed-off-by: Flavio Ceolin <flavio@hubble.com>
github-actions
Bot
requested review from
HongNguyen635,
buckleypaul and
hunterhubble
| #define WAIT_SYMBOL_US (HUBBLE_WAIT_SYMBOL_US + 140) | ||
|
|
||
| /* Max time for semaphore symbol to wait before failing. */ | ||
| #define WAIT_SYMBOL_TIMEOUT_US K_USEC(2 * (WAIT_SYMBOL_OFF_US + WAIT_SYMBOL_US)) |
There was a problem hiding this comment.
is the 2 here arbitrary? Or is it related to the 2 uses in HUBBLE_SAT_TRANSMISSION_TIMEOUT_S?
There was a problem hiding this comment.
I'm not sure how to feel about the use of constant 2 through the code. When we decide to change it to another number later, we'll have to go through the port and change each. Would it makes sense if we define something for it, e.g. SYMBOL_TIMEOUT_FACTOR or something like that...
There was a problem hiding this comment.
it is arbitrary, just two times the expected time, could be even less.
Let me re-struct it and make it consistent
| #define NRF_DPPIC NRF_DPPIC10 | ||
| #define RADIO_ENABLE_TX_ON_CC0_PPI 9U |
There was a problem hiding this comment.
another weird case from the formatter :(
There was a problem hiding this comment.
yep, super annoying. I can try to revert it and see if CI complains
| return _sl_status_to_errno(status); | ||
| } | ||
|
|
||
| if (ret != 0) { |
There was a problem hiding this comment.
should we preserve the error from k_sem_take here?
Maybe:
return (ret != 0) ? ret : _sl_status_to_errno(status);
But I'm not sure if this makes it hard to read / maintain.
| #include <hubble/port/sat_radio.h> | ||
| #include <hubble/sat/packet.h> | ||
| #include <hubble/port/sat_radio.h> |
| #error "Device not supported" | ||
| #endif | ||
|
|
||
| #define _TIME_S_TO_TICK(_time_s) ((_time_s * 1000U) / portTICK_PERIOD_MS) |
There was a problem hiding this comment.
I'm a bit curious on is this preferred over pd_MS_TO_TICKS( )? I recall something about using pd_MS_TO_TICKS have some safety guard like promotion and type cast.
| #define _TIME_US_TO_TICK(_time_us) ((_time_us / 1000U) / portTICK_PERIOD_MS) | ||
| #define _TIME_S_TO_TICK(_time_s) ((_time_s * 1000U) / portTICK_PERIOD_MS) |
There was a problem hiding this comment.
I kind of feeling something off about this so I asked Claude and checked with ESP-IDF source:
#define portTICK_PERIOD_MS ( ( TickType_t ) 1000 / configTICK_RATE_HZ )
-> portTICK_PERIOD_MS = 1000 / 100 = 10
where tick rate Hz is 100 by default. I also doubled check the build folder and the default of CONFIG_FREERTOS_HZ is indeed 100.
So say we have symbol off = 800 us -> (800 / 1000U) / 10 = 0
or symbol on = 8000 us -> ( 8000 / 1000 ) / 10 = 0
So either case it still fails.
There was a problem hiding this comment.
Yep, good point, we should have at least 1 tick as min.
2 participants
Add timeouts to start transmissions and between symbols.