Conversation
|
Many thanks for this contribution! A few things we'll need before merging:
Checklist:
I suspect ChatGPT can do all this pretty automatically. Alternatively, if you'd prefer, you could delegate this to me and I can merge your relevant changes onto my branch. Thanks again for pushing this forward! Best regards, |
|
Hi Terry, Yes, you guessed right. I asked Claude Code to implement class C on the latest version based on the work you had already carried out on branch issue208-578a. I understand that you'd like to keep the history of that work. From what I see there are 36 commits in that branch that are not in v5.0.1 (diverged at commit 8d378ea). Some of which don't seem to concern class C like commit b17b4c7. Is that normal? I'm happy to help see this through. That being said, if you think it's more appropriate that you take over from here, I have no issue that. Best regards, |
|
Practically speaking, it will happen sooner if you do the next step. I suspect you run claude less conservatively than I do, I watch it carefully at each step, and that's time consuming. And my time is at a premium. I also use Claude Code. I believe that if you tell Claude: "OK, that works. Now we need to rebase Terry's branch issue208-578a on the real v5.0.1; then we need compare that to this pull request and figure out whan changed and add those changes to branch isseu208-578a. Preserve all the commit history from branch issue208-578a (do not squash commits)," it will probably work. Anyway, it's easy to try. If claude screws up, it's not very hard to do it manually. This would make it much easier to review. Once that's done, one of us can ask Claude to review the documentation and make the appropriate updates, and to create compilation test cases. |
|
By the way -- the commits in the branch that are not class C were other bugs that I found in my review. My intention was to rebase once I got done -- keeping commit history and having lots of commits help you do this. If we can get to the point of having a branch with the full commit history, we can then ask claude to rearrange the order of the commits to keep non-Classs-C things separate. It's possible that I already cherry-picked non-class-C things. I think Claude will figure that out. We've been doing group Claude sessions in The Things Network New York meetings recently; if you wanted to join a Teams meeting we could do this jointly. Or just continue via email like this. Thing is I need to watch it working to figure out what needs to be in CLAUDE.md and other project instructions to let it do the best job -- it's hard to do that via comments in a commit history. |
|
Ok, so all commits even the ones that are not Class C related should be rebased on v5.0.1 right? I'll go ahead and try this with the help of your instructions. I'm also interested in joining one of your Claude group sessions. Could you please send me the details via mail (in commit)? |
|
I would first ask claude to rebase https://github.com/mcci-catena/arduino-lmic/tree/issue208-578a onto master. (This will remove all changes that I already incorporated into 5.0.1.) Then ask it to compare the result to what you have, and incorporate fixes from your branch into the new branch. Code is pretty capable; if you do this from the same session (use claude --resume) that generated the changes, it will figure out what you're doing and fill in the blanks. You can make a PDF of this email thread and ask Claude to read it for context. Best way to join meetings is to get into the slack for thethings.nyc -- normally that needs an email address. My address is already published: terry@thethings.nyc -- if you email me, I'll add you to the slack. |
…data length of aes_appendMic()
…om secure element layer
This is a breaking change, because clients previously could use `LMIC.rxtime` to schedule a receive. This no longer works; `LMIC.nextRxTime` is used to schedule the receive, and now `LMIC.rxtime` only is used to report the time of last receive. But in addition, this adds a new configuration variable, `LMIC_ENABLE_class_c`, a new API for run-time enabling (if configured in), and support for starting class C receives at the appropriate times. This seemed to require an additional receive buffer, to avoid collisions; and therefore client code that depends on results showing up in LMIC.frame[] may be rudely surprised. os_radio(RADIO_RST) now inserts a 1ms delay to allow the radio to recover.
os_radio_isTxActive() and os_radio_isRxActive() were missing explicit returns for the FALSE path. Certainly a bug. Found by GCC -Wall.
- Add os_radio_v2() for async radio operations with job callbacks - Add RADIO_RXON_C mode handling for continuous RX on RX2 (both drivers) - Add os_radio_reset() helper function - Add radio state tracking (LMIC.radio.state) - Use LMIC.radio.pFrame for Class C buffer in RX path - Fix hal_waitUntil -> lmic_hal_waitUntil in SX127x driver - Fix Class C confirmed downlink ACK: call engineUpdate() to schedule ACK uplink after receiving confirmed downlink in continuous RX mode - Rewrite processRxCDnData() to properly handle Class C frame processing This completes Class C support for both SX127x and SX126x radios. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add Class C compile tests for ESP32 (US915, EU868, debug), AVR (US915), SAMD (US915, EU868), and STM32L0 (US915, EU868) targets - Fix conditional compilation bug in processRxCDnData() that referenced LMIC.classC.frame when Class C was disabled - Note: SX1262 Class C tests disabled due to pre-existing radio driver bug Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update README.md Features section to mention Class C - Add "Enabling Class C" configuration section to README.md - Add TOC entry for Class C configuration - Note SX1262 Class C compilation issues in README.md - Rewrite doc/CLASS-C.md from working notes to full documentation including API reference, usage examples, and build instructions - Update doc/README.md to reference CLASS-C.md documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
f816eab to
96bdabf
Compare
|
I proceeded as instructed. Here is the result after force pushing the changes. An issue was found with the SX126x driver. Should that be fixed in this PR too? |
|
This is very helpful! Now I can see from the log that we're also merging the secure element changes -- that's not a bad idea at all. I think it also merges the doxygen setup. Re the bug in the SX126x driver: I think it's ok to push the change, as long as an issue is created and the commit message for the fix starts with "Fix #{issue-num}: ..." so that the issue will be autoclosed when we merge.
Since the main doc is a word doc, I may use CoWork to try to update the word doc. (What happens with AI is that you can do so much more, that the work of prompting expands to fill the available time. We should ask Claude CoWork to study all of the documentation, and reorganize and unify so that it's easier for people to get started; and to look at the issue history to write up a troubleshooting guide. Although for troubleshooting, people probably should just ask Claude at this point.) Thanks! |
|
Looks like AVR failed, probably due to code size. We could probably ask Claude Code to look into it. We might need to make a config change for the AVR CI builds.. |
Fix mcci-catena#1021 SX126x driver fixes: - Fix LMIC.radio_txpow -> LMIC.radio.txpow (member doesn't exist) - Replace os_aes(AES_ENC, ...) with LMIC_SecureElement_aes128Encrypt() to match SX127x driver and use proper secure element API - Add missing #include for lmic_secure_element_api.h - Add randseed[] and randround variables for proper CSPRNG like SX127x - Remove redundant LMIC.radio.txpow assignment (already set by lmic.c) CI regression tests: - Add SX1262 basic tests (US915, EU868) - Add SX1262 Class C tests (US915, EU868) - Remove obsolete comment about SX1262 bug Documentation: - Remove SX1262 limitation notes from README.md and CLASS-C.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Created issue #1021 and pushed the fix |
Strange... The tests pass locally on my end |
|
I had Claude look into the AVR failing test issue:
How do you want to proceed? |
|
Hey all, a few things: A. Bumping this, as it looks like it's super close to getting merged, which would be fantastic! EDIT: After writing everything below, I realized...are we supposed to be using case EV_RXCOMPLETE:
Serial.print("Received ");
Serial.print(LMIC.dataLen);
Serial.println(" bytes");
if (LMIC_txrxFlags_isClassC(LMIC.txrxFlags)) {
Serial.println("(Class C downlink)");
}
// Process received data
for (int i = 0; i < LMIC.dataLen; i++) {
Serial.print(LMIC.frame[LMIC.dataBeg + i], HEX);
Serial.print(" ");
}
Serial.println();
break;But everything works totally fine if we instead use: Serial.print(LMIC.classC.frame[LMIC.dataBeg + i], HEX);Original comment: B. On that note, I have been testing this branch and appear to have found a bug(?) with the class C message decoding. Unfortunately, I don't have a great understanding of the inner workings here, so pardon my ignorance. If I should move this to an issue, let me know; I'm not a big contributor on GH. In os_copyMem(LMIC.frame, LMIC.classC.frame, LMIC.dataLen);However, when processing the downlink: if (LMIC.dataLen > 0 && decodeFrame()) {
...
...
xref2u1_t d = LMIC.radio.pFrame;
...This leads to the payload being incorrect. That's the part I'm unsure about. I'm hoping it'll be a quick fix for @Beetix, as it causes the payload data you process in a RX_COMPLETE callback to be incorrect. I debugged the frames just before decoding, but I don't know enough to know why, even though As a temporary workaround, I added the following in ...
if (LMIC.dataLen > 0) {
os_copyMem(LMIC.frame, LMIC.classC.frame, LMIC.dataLen);
LMIC.radio.pFrame = LMIC.frame; // <--
}
...Of course, after adding the line above, debugging the frames is as expected: And my payload is now decoded correctly. Instead of getting random integers from my payload (due to it still being encrypted?), I get my payload value of Otherwise, this branch and class C work amazingly! Thanks for all the work here. |
After copying classC.frame to LMIC.frame in processRxCDnData(), radio.pFrame still pointed to the Class C buffer. Since decodeFrame() uses radio.pFrame, the payload was not correctly decrypted. Redirect pFrame to LMIC.frame after the copy so decodeFrame() operates on the correct buffer. Fixes issue reported in PR mcci-catena#1019 (issuecomment-4044044675). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Hi @ychlc, Your finding is correct! Thank you for your contribution to this PR. I've commited and pushed the modification you propose. I'm still waiting for @terrillmoore to respond so it can me merged. Fingers crossed! |
|
Hi all,
I’m hoping to review and push this along this weekend. Sorry for the delay.
…--Terry
From: Benjamin Freeman ***@***.***>
Sent: Friday, March 13, 2026 18:38
To: mcci-catena/arduino-lmic ***@***.***>
Cc: Terry Moore ***@***.***>; Mention ***@***.***>
Subject: Re: [mcci-catena/arduino-lmic] Add LoRaWAN Class C support (PR #1019)
<https://avatars.githubusercontent.com/u/10374047?s=20&v=4> Beetix left a comment (mcci-catena/arduino-lmic#1019) <#1019 (comment)>
Hi @ychlc <https://github.com/ychlc> ,
Your finding is correct! Thank you for your contribution to this PR. I've commited and pushed the modification you propose.
I'm still waiting for @terrillmoore <https://github.com/terrillmoore> to respond so it can me merged. Fingers crossed!
—
Reply to this email directly, view it on GitHub <#1019 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AETP73YPL4435R6XQ2TU6EL4QSEUHAVCNFSM6AAAAACSVTUW7CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANJYGQ2TGMRSHA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AETP733NJ5NFYPFLUBE22C34QSEUHA5CNFSM6AAAAACSVTUW7CWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTXR44KOY.gif> Message ID: ***@***.*** ***@***.***> >
|
Implement Class C continuous downlink reception capability:
Class C is disabled by default. Enable with LMIC_ENABLE_class_c=1. After OTAA join, call LMIC_enableClassC(1) to start continuous reception.
Tested on ESP32 (M5Stack Core2) with SX1276 radio and EU868 band.