Removing software RX timeouts in RX chain #22

hasnainvirk · 2018-06-20T09:41:43Z

Both in FSK and LoRa mode, we do not rely on software timers anymore.
This makes our timing precise and enables us to consume less power.
It also gives us flexibility which we need for future deep sleep
venture as Mbed OS timeout class may lock the sleep manager if not using
low power timers.

hasnainvirk · 2018-06-20T09:42:12Z

Please review @AnttiKauppila @kjbracey-arm

kjbracey · 2018-06-20T09:57:55Z

SX1272/SX1272_LoRaRadio.cpp

@@ -904,6 +896,11 @@ void SX1272_LoRaRadio::receive(uint32_t timeout)
                              RF_RXCONFIG_AGCAUTO_ON |
                              RF_RXCONFIG_RXTRIGER_PREAMBLEDETECT);

+            write_to_register(REG_RXTIMEOUT2, _rf_settings.fsk.rx_single_timeout <= 255 ?


If going to have any special code for this range check, it should produce some sort of diagnostic. The system will fail if you need a timeout bigger than 255 and you can't provide it.

Either the condition won't happen, so it's not worth having the range check, or it might, in which case you want to print something while limping on with a not-big-enough-but-best-we-can-do timeout.

(Probably once only - would be nice if there was a tr_error_once macro. Feature request?)

The RX_TIMEOUT is being used for FSK only and with recommended error cushion of 10ms, number of bytes are never going to be more than 255. 100ms value for error cushion is not recommended.

So if it's never going to be more than 255, you don't need the check.

yeah, just my paranoia .. The types chosen by original developers are weird. I want to have the test, may be I am being a bit too defensive here :) but you taught me that, didn't you ?

Paranoia is justified I think here, but this particular "quietly saturate" reaction if they really are out to get you isn't helpful.

kjbracey · 2018-06-20T09:59:57Z

SX1272/SX1272_LoRaRadio.cpp

+                    // We can have a snapshot of RSSI here as at this point it
+                    // should be more smoothed out.
+                    _rf_settings.fsk_packet_handler.rssi_value = -(read_register(REG_RSSIVALUE) >> 1);
+                    _rf_settings.fsk_packet_handler.afc_value = (int32_t)(double)(((uint16_t)read_register(REG_AFCMSB) << 8) |


Generally you should be trying to reduce or totally eliminate use of floating point, if you're aiming at low-end devices. At least use float rather than double given that a lot of chips we're on have single-precision hardware only.

Yeah I will change everything with float precision. Good point.

kjbracey · 2018-06-20T10:00:45Z

SX1276/SX1276_LoRaRadio.cpp

@@ -848,29 +844,24 @@ void SX1276_LoRaRadio::standby( void )
 * and finally a DIO0 interrupt let's the state machine know that a packet is
 * ready to be read from the FIFO
 */
-void SX1276_LoRaRadio::receive(uint32_t timeout)
+void SX1276_LoRaRadio::receive(uint32_t /**timeout**/)


Does this make it a Doxygen comment? Is that deliberate?

I am not sure if this makes a doxygen comment. But it is deliberate. I just wanted to emphasis that this parameter is not being used. We will deprecate the APIs in a separate commit.

If you aren't consciously doing something for Doxygen, then probably best not to use the special /** Doxygen syntax. Then we won't have to worry about how it will interpret it.

Both in FSK and LoRa mode, we do not rely on software timers anymore. This makes our timing precise and enables us to consume less power. It also gives us flexibility which we need for future deep sleep venture as Mbed OS timeout class may lock the sleep manager if not using low power timers.

We shouldn't use double precision as most of te embedded processors may not support it.

hasnainvirk · 2018-06-26T08:28:31Z

@kjbracey-arm Can you please review ?

hasnainvirk · 2018-06-26T08:37:13Z

@AnttiKauppila Please review.

kjbracey · 2018-06-26T08:35:12Z

SX1276/SX1276_LoRaRadio.h

     *  @param timeout       Reception timeout [ms]
     *
     */
-    virtual void receive(uint32_t timeout);
+    MBED_DEPRECATED_SINCE("mbed-os-5.10", "receive(uint32_t) will be deprecated. "


You can't warn about deprecation in the future. It's either deprecated or it isn't. This is saying "I'm telling you that I'm going to tell you that you shouldn't use this in the future, but I'm not telling you not to use it now", which doesn't make a lot of sense.

Anyway, a deprecation notice isn't appropriate for this outside-mbed OS driver implementation. It has to provide the current method, which is receive(timeout), but is providing receive() ready for some future version of LoRaRadio. (Which I still think could arrive by 5.10, once all drivers are updated).

receive(uint32_t) API in the LoRaRadio class (base class for the radio drivers) should not take any argument as we decided to take hardware timers for RX timeout interrupts instead of software timers. It is being refactored to receive(void). This is an API change, but as it is not an application interface, we will not put a deprecation notice. Only user of this API is our stack (LoRaPHY layer) which has been updated accordingly. Actual driver comes out of the tree and a PR is open there to update the drivers: ARMmbed/mbed-semtech-lora-rf-drivers#22 In addition to this an internal API belonging to LoRaPHY class is refactored. set_rx_window(parameters) is refactored to handle_receive(void) which is more consistent with handle_send().

We are deprecating receive(uint32_t) API in favour of receive(void) API because we are ditching software timeout timers in the driver for RX chain.

Rx timeout in symbols cannot become more than 255 as the preamble length is fixed. For diagonostics purposes we add an MBED_ASSERT.

hasnainvirk · 2018-06-26T13:03:47Z

@kjbracey-arm Please review again.

hasnainvirk requested review from AnttiKauppila and kjbracey June 20, 2018 09:41

hasnainvirk changed the title ~~Removing~~ Removing software RX timeouts in RX chain Jun 20, 2018

kjbracey reviewed Jun 20, 2018

View reviewed changes

hasnainvirk changed the title ~~Removing software RX timeouts in RX chain~~ [DO NOT MERGE] Removing software RX timeouts in RX chain Jun 20, 2018

Hasnain Virk added 2 commits June 21, 2018 11:59

Changing double precision to float

7325b3e

We shouldn't use double precision as most of te embedded processors may not support it.

hasnainvirk force-pushed the recv_timeout_issue branch from 12368dd to 7325b3e Compare June 21, 2018 09:00

hasnainvirk changed the title ~~[DO NOT MERGE] Removing software RX timeouts in RX chain~~ Removing software RX timeouts in RX chain Jun 26, 2018

kjbracey reviewed Jun 26, 2018

View reviewed changes

hasnainvirk mentioned this pull request Jun 26, 2018

LoRaWAN: Refactoring LoRaRadio::receive(uint32_t) API ARMmbed/mbed-os#7335

Merged

Hasnain Virk added 2 commits June 26, 2018 16:02

Deprecating receive(uint32_t) API

d135987

We are deprecating receive(uint32_t) API in favour of receive(void) API because we are ditching software timeout timers in the driver for RX chain.

Adding assert if rx timeout is out of range

bccd3fc

Rx timeout in symbols cannot become more than 255 as the preamble length is fixed. For diagonostics purposes we add an MBED_ASSERT.

hasnainvirk force-pushed the recv_timeout_issue branch from ea21131 to bccd3fc Compare June 26, 2018 13:03

kjbracey approved these changes Jun 26, 2018

View reviewed changes

hasnainvirk merged commit 1a5f2fc into master Jun 26, 2018

mattbrown015 mentioned this pull request Jan 13, 2020

FRF Calculation Inaccuracy #50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Removing software RX timeouts in RX chain #22

Removing software RX timeouts in RX chain #22

Uh oh!

hasnainvirk commented Jun 20, 2018

Uh oh!

hasnainvirk commented Jun 20, 2018

Uh oh!

kjbracey Jun 20, 2018

Uh oh!

hasnainvirk Jun 20, 2018

Uh oh!

kjbracey Jun 20, 2018

Uh oh!

hasnainvirk Jun 21, 2018

Uh oh!

kjbracey Jun 21, 2018

Uh oh!

kjbracey Jun 20, 2018

Uh oh!

hasnainvirk Jun 20, 2018

Uh oh!

kjbracey Jun 20, 2018

Uh oh!

hasnainvirk Jun 20, 2018

Uh oh!

kjbracey Jun 20, 2018

Uh oh!

hasnainvirk commented Jun 26, 2018

Uh oh!

hasnainvirk commented Jun 26, 2018

Uh oh!

kjbracey Jun 26, 2018

Uh oh!

hasnainvirk commented Jun 26, 2018

Uh oh!

Uh oh!

Removing software RX timeouts in RX chain #22

Removing software RX timeouts in RX chain #22

Uh oh!

Conversation

hasnainvirk commented Jun 20, 2018

Uh oh!

hasnainvirk commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented Jun 26, 2018

Uh oh!

hasnainvirk commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented Jun 26, 2018

Uh oh!

Uh oh!