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

library: lr1110: Add library for LR1110 support #458

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

ethanzhouyc
Copy link

Add lr1110 library and two related example applications. This library uses Seeed Studio's code as user space, built upon Tock, to provide support for the LR1110 LoRa chip on Wio-WM1110 development board. Applications could use the library to collect sensor data, join TTN, perform Wi-Fi scans, and send data to TTN for geolocation.

Copy link
Contributor

@alistair23 alistair23 left a comment

Choose a reason for hiding this comment

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

Cool! Soon Tock will have so much LoRa support!

Comment on lines 82 to 84
#define LORAWAN_DEVICE_EUI "70B3D57ED00650D9"
#define LORAWAN_JOIN_EUI "901AB1F40E1BCC81"
#define LORAWAN_APP_KEY "3356A7047ECF1F2F78C72AE9B1635BC1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these your keys? You want to keep the keys secret, you probably want to have this default to 0 and users can set them

Comment on lines 14 to 18
void hal_clock_init(void)
{
//nrf_drv_clock_init();
//nrf_drv_clock_lfclk_request( NULL );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These empty functions should just be removed instead of commented out

/**
* \file region_us_915.c
*
* \brief region_us_915 abstraction layer implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this file need to be modified?

@@ -0,0 +1,21 @@
# LoRa Basics Modem LoRaWAN Class A/C example
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be clear that this only targets the Wio WM1110

@bradjc
Copy link
Contributor

bradjc commented Jul 23, 2024

I'm working on putting the finishing touches on this pr

@ethanzhouyc
Copy link
Author

@bradjc Maybe we should keep wifi_helpers.c in the src_changed folder for now. Changes in these lines make sure that the app does not return error when the board sometimes scans too many Wi-Fi access points.

@ethanzhouyc
Copy link
Author

Great to see the library been further cleaned up!

@bradjc
Copy link
Contributor

bradjc commented Jul 24, 2024

@bradjc Maybe we should keep wifi_helpers.c in the src_changed folder for now. Changes in these lines make sure that the app does not return error when the board sometimes scans too many Wi-Fi access points.

Good point. I think I was thinking we should change WIFI_MAX_RESULTS, but that isn't straightforward easy, and could run into the same issue.

@bradjc
Copy link
Contributor

bradjc commented Jul 24, 2024

I wasn't able to actually get this to work, and I think I've traced the issue back to changes to how the alarm_read function works. I forgot to compile the library when I tested so I still need to verify this, but this is my working diff:

diff --git a/lr1110/lr1110/src_changed/smtc_hal_rtc.c b/lr1110/lr1110/src_changed/smtc_hal_rtc.c
index 39cc844e..15cb80ac 100644
--- a/lr1110/lr1110/src_changed/smtc_hal_rtc.c
+++ b/lr1110/lr1110/src_changed/smtc_hal_rtc.c
@@ -10,7 +10,7 @@
 // static uint32_t rtc_2_tick_diff = 0;

 // static void rtc_2_handler( nrf_drv_rtc_int_type_t int_type )
-// {
+// {
 //     if( int_type == NRF_DRV_RTC_INT_OVERFLOW )
 //     {
 //         rtc_2_tick_diff += RTC_2_MAX_TICKS;
@@ -43,17 +43,47 @@ uint32_t hal_rtc_get_time_ms( void )
     return timeInms / 10;
 }

+static uint32_t ticks_to_100us(uint32_t ticks) {
+  // `ticks_to_ms`'s conversion will be accurate to within the range
+  // 0 to 1 milliseconds less than the exact conversion
+  // (true millisecond conversion - [0,1) milliseconds).
+
+  uint32_t frequency;
+  libtock_alarm_command_get_frequency(&frequency);
+
+  uint32_t seconds = (ticks / frequency);
+  uint32_t hundredus_per_second = 10000;
+
+  // Calculate the conversion of full seconds to ticks.
+  uint32_t hundredus = seconds * hundredus_per_second;
+
+  // To get conversion accuracy within 1 millisecond, the conversion
+  // must also convert partial seconds.
+  uint32_t leftover_ticks = ticks % frequency;
+
+  // This calculation is mathematically equivalent to doing:
+  //
+  //   `leftover_ticks` / (`frequency` / `hundredus_per_second`)
+  //
+  // This is taking the remaining unconverted ticks (leftover_ticks)
+  // and dividing by the number of ticks per millisecond
+  // (`frequency` (ticks per second) / `1000` hundredus per second)
+  // The division is done this way because of the same argument in
+  // `ms_to_ticks`.
+  hundredus += (leftover_ticks * hundredus_per_second) / frequency;
+
+  return hundredus;
+}
+
 uint32_t hal_rtc_get_time_100us( void )
 {
     // double temp = nrf_drv_rtc_counter_get( &rtc_2 ) + rtc_2_tick_diff;
     // return temp * RTC_2_PER_TICK * 10;
-
+
     uint32_t time = 0;
     uint32_t frequency = 0;
     libtock_alarm_command_read(&time);
-    libtock_alarm_command_get_frequency(&frequency);
-    uint32_t timeIn100us = (time * 10000) / frequency;
-    return timeIn100us;
+    return ticks_to_100us(time);
 }

 // uint32_t hal_rtc_get_max_ticks( void )
@@ -64,7 +94,7 @@ uint32_t hal_rtc_get_time_100us( void )
 void hal_rtc_wakeup_timer_set_ms( const int32_t milliseconds )
 {
     libtocksync_alarm_delay_ms(milliseconds);
-
+
     // uint32_t temp = 0;
     // float temp_f = milliseconds;
     // temp_f = temp_f / RTC_2_PER_TICK + 1;
@@ -82,5 +112,5 @@ void hal_rtc_wakeup_timer_set_ms( const int32_t milliseconds )

 // void hal_rtc_wakeup_timer_stop( void )
 // {
-
+
 // }

@alistair23
Copy link
Contributor

@bradjc Maybe we should keep wifi_helpers.c in the src_changed folder for now. Changes in these lines make sure that the app does not return error when the board sometimes scans too many Wi-Fi access points.

Good point. I think I was thinking we should change WIFI_MAX_RESULTS, but that isn't straightforward easy, and could run into the same issue.

Can you upstream the fix?

bradjc added 2 commits July 26, 2024 10:48
If frequency is high, then `leftover_ticks * 1000` can exceed a 32 bit
number.
In case the timer has a high frequency.
@bradjc
Copy link
Contributor

bradjc commented Jul 26, 2024

Ok I can confirm that better handling the ticks to 100us conversion fixed the issue.

@bradjc
Copy link
Contributor

bradjc commented Aug 5, 2024

I need to work on the build error of course but otherwise I think I have this PR to a state where it is reasonably mergeable.

/*!
* @brief Defines the application data transmission duty cycle. 60s (changed to 3s), value in [s].
*/
#define APP_TX_DUTYCYCLE 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this how often data is uploaded?

If it is this is really low! This breaks TTN usage policy and I think breaks FCC laws in America

Copy link
Author

Choose a reason for hiding this comment

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

I remembered it is how often the app makes an attempt to transmit data to TTN, and if the transmission succeeds, the duty cycle will stop. I agree that we should make it a larger number, but if it's too large, like 60, it might fail the app.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's way too low then, this needs to be at least 60 minutes

Copy link
Author

Choose a reason for hiding this comment

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

Since this is only for a test app, the impact should be minimal here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not minimal impact, people will be banned from TTN if they run the test app and get caught.

Not to mention it's possible a crime in some countries

examples/tests/lr1110/lorawan/main_lorawan.c Outdated Show resolved Hide resolved
examples/tests/lr1110/lorawan/main_lorawan.c Show resolved Hide resolved
examples/tests/lr1110/wifi_scan/README.md Outdated Show resolved Hide resolved
examples/tests/lr1110/wifi_scan/main_geolocation_wifi.c Outdated Show resolved Hide resolved
SEEED_DIR = $(LR1110_DIR)/seeed

# include changed headers and parameters
override CFLAGS += -I$(LR1110_DIR)/inc_changed -DREGION_US_915 -DRP2_103 -DTASK_EXTENDED_2 -DLR11XX -DLR11XX_TRANSCEIVER
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I think the region setting needs a better solution

Copy link
Author

Choose a reason for hiding this comment

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

Ideally we should prompt users to input their region code, but we discussed to agree that this approach will be sufficient for the current stage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Who discussed?

This isn't sufficient as you don't actually allow people to change it. It's set here and in the code and completely undocumented

Copy link
Author

Choose a reason for hiding this comment

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

We will think about that a bit more

Comment on lines +1 to +3
#include "lr1_stack_mac_layer.h"
#include "lr1mac_utilities.h"
#include "lorawan_api.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, what about other regions?

Copy link
Author

Choose a reason for hiding this comment

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

It will be hard to test in other regions too

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to test it, but the library supports a range of regions which they have tested. You just need to allow users to set the region

lr1110/lr1110/src_changed/smtc_hal_mcu.c Show resolved Hide resolved
Comment on lines +7 to +10
- MW_DBG_TRACE_ERROR( "Wi-Fi scan result size exceeds %u (%u)\n", max_nb_results, nb_results );
- return false;
+ // Just use the number we received that can fit in our results array.
+ nb_results = max_nb_results;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Author

Choose a reason for hiding this comment

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

The app returns an error if more than 5 results are detected, which sometimes occurs when there are many Wi-Fi access points nearby. This update retains all original functionality while preventing potential errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

That should go upstrem then

Copy link
Author

Choose a reason for hiding this comment

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

Seeed Studio is not going to upstream this. It is caused by integrating their code with Tock OS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then that's a bug that should be fixed, not just hacked around

Comment on lines +29 to +38
+ // For some reason we get a spurious RX_TIMEOUT interrupt sometimes.
+ // Handling it causes the stack to break, but clearing it causes the
+ // stack to work.
+ // re-get the irq status if RX_TIMEOUT happens (current workaround)
+ if( rp_task_type == RP_TASK_TYPE_WIFI_RSSI) {
+ if(rp->status[rp->radio_task_id] == RP_STATUS_RX_TIMEOUT) {
+ rp_irq_get_status( rp, rp->radio_task_id );
+ // return;
+ }
+ }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be upstreamed, not just hacked around

Copy link
Author

Choose a reason for hiding this comment

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

It's the only workaround we could find. We spent lots of time debugging this and it's hard to resolve

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue opened against the original library?

Copy link
Author

Choose a reason for hiding this comment

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

No, their code will work fine on their OS stack, but we are integrating their code into Tock OS here.

Comment on lines 84 to 86
#define LORAWAN_DEVICE_EUI "70B3D57ED00650D9"
#define LORAWAN_JOIN_EUI "901AB1F40E1BCC81"
#define LORAWAN_APP_KEY "3356A7047ECF1F2F78C72AE9B1635BC1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an FYI, the key is still here. So make sure you revoke it from TTN

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.

3 participants