LoRa improvements #24649
LoRa improvements #24649
Conversation
|
Thanks for pulling this out of the LoRaWAN PR, it's going the be helpful for #24616. Would it make sense to split the RTC code into a separate source file right away since you're rewriting most of it anyway? Saves me from doing it later. :) I verified the assumptions in the new implementation against the documentation and one of the other board implementations in LoRaMAC-Node and it looks good to me. |
|
@andysan would you mind approving if you are happy with this? |
There was a problem hiding this comment.
If I am correct then Rtc in loramac-node is used only for implementing Timer in loramac-node. So now we have:
Timer (loramac-node) <-> Rtc (loramac-node) <-> k_timer (Zephyr)
In long term we should think about implementing Timer API instead of Rtc API, which will require less bloat. This however requires changes in loramac-node repository, because it is not possible now.
There was a problem hiding this comment.
This is really just an observation, but couldn't we just define ticks in LoRaMAC-node as being milliseconds and do away with all of the annoying time conversions? That way, tick2ms and ms2tick suddenly become unit operations. We would probably have to return k_ticks_to_ms_ceil32(1) as the minimum timeout in that case though.
drivers/lora/sx1276.c
Outdated
| { | ||
| return counter_us_to_ticks(dev_data.counter, (milliseconds / 1000)); | ||
| saved_time = k_ms_to_ticks_ceil32(k_uptime_get_32()); |
There was a problem hiding this comment.
If we want to deal with ticks anyway, why don't use the k_uptime_ticks API instead and store the saved time as an s64_t? The documentation actually states that k_uptime_get_32 uses the full precision clock, so there is normally no benefit to using the 64-bit version.
There was a problem hiding this comment.
We could most probably get rid of ticks and only deal with real time. The k_timeout_t supports also us or ns timevalues.
There was a problem hiding this comment.
This is really just an observation, but couldn't we just define ticks in LoRaMAC-node as being milliseconds and do away with all of the annoying time conversions? That way, tick2ms and ms2tick suddenly become unit operations. We would probably have to return k_ticks_to_ms_ceil32(1) as the minimum timeout in that case though.
The idea is not to change the LoRaMac-node repository. If we start doing this then for sure the delta between upstream and downstream will increase and I don't want to have that :) We should live with what the repository provides. If we want to change something then we should do that in upstream repo and inherit here.
There was a problem hiding this comment.
We could most probably get rid of ticks and only deal with real time. The k_timeout_t supports also us or ns timevalues.
Sorry no. It is like abusing the API exposed by LoRaMac-node repo. We already did that once with Rtc/timer but there we had no other choice. But for this case, since we have an API for getting the ticks, I'd like to stick to it.
There was a problem hiding this comment.
If we want to change something then we should do that in upstream repo and inherit here.
Maybe post an issue on the Semtech repo and ask if they are receptive to integrating with zephyr?
There was a problem hiding this comment.
It will help with decisions down the road about how much to use the underlying zephyr kernel API's.
There was a problem hiding this comment.
My thought is Semtech may have an interested to make their stack available across a larger base of devices and reduce their maintenance.
There was a problem hiding this comment.
My thought is Semtech may have an interested to make their stack available across a larger base of devices and reduce their maintenance.
But Zephyr is not the only user of their stack...
There was a problem hiding this comment.
What I meant by defining ticks as ms wouldn't require a change in the LoRaMAC code. The only change it would involve would be the way we expose ticks to the library. It's no different from a real system that just happens to define a tick as 1 ms. This would avoid the weird conversions where we go from microseconds to ticks, and then from ticks to microseconds when scheduling alarms.
If we do that, RtcMs2Tick(x) and RtcTick2Ms(x) would just return x. Calls that take a tick as an argument could just rely on a tick in LoRa being defined as 1ms, or use RtcTick2Ms to document the fact (the call would get inlined).
There was a problem hiding this comment.
For the record, I've switched to k_uptime_get_32() API
|
Can someone comment on the minimum resources required to run this stack on zephyr? Would the stm32l04 mcu inside the Murata lora module be able to run the lora stack on zephyr? I am considering to build a new LoRaWAN firmware stack for the Arduino MKR WAN 1310 that runs on the Murata module. The module currently runs a version of the Semtech AT Slave application. However, Arduino has not updated it with the more recent 4.4.2 firmware releases from Semtech so it is terribly out of date and lacks support for time synchronization and fuota. |
Mani-Sadhasivam
force-pushed
the
lora-timer
branch
from
33fbceb
to
cb75ca8
Compare
The RTC/Counter implementation doesn't fit for the upcoming LoRaWAN as most of the Counter drivers in Zephyr works with 1s granularity which is not enough for LoRaWAN stack. So, k_timer calls are used in place of Counter's alarm and k_uptime_get() APIs are used in place of Counter's time keeping. While at it, lets also fixup the broken alarm implementation. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
The LoRa APIs are expected to undergo some change as the usecase get's increased. So let's mark it as experimental until the APIs got stabililized. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Mani-Sadhasivam
force-pushed
the
lora-timer
branch
from
5018b64
to
13471b7
Compare
|
@mniestroj can you please take another look? |
Get rid of legacy timeout API and move to new timeout API for LoRa. This involves changes to API, SX1276 driver and sample application. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
RtcTick2Ms function is required for upcoming LoRaWAN support. Hence, add definition for it. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Mani-Sadhasivam
force-pushed
the
lora-timer
branch
from
13471b7
to
5e07b81
Compare
This PR is a spinoff from #23664
Signed-off-by: Manivannan Sadhasivam mani@kernel.org