-
-
Notifications
You must be signed in to change notification settings - Fork 9
Modify RNodeConfigValidator and Test to allow SF5 and SF6 #378
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
Modify RNodeConfigValidator and Test to allow SF5 and SF6 #378
Conversation
Greptile OverviewGreptile SummaryUpdated the spreading factor validation to allow SF5 and SF6 in addition to SF7-12. Changes:
Analysis: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant UI
participant RNodeConfigValidator
participant ValidationResult
User->>UI: Enter SF value (e.g., "5", "6")
UI->>RNodeConfigValidator: validateSpreadingFactor(value)
alt value is blank
RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=true)
Note over RNodeConfigValidator: Allow empty while typing
else value is not a number
RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=false, "Invalid number")
else value < MIN_SF (5)
RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=false, "Must be >= 5")
else value > MAX_SF (12)
RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=false, "Must be <= 12")
else valid range (5-12)
RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=true)
end
ValidationResult->>UI: Return validation result
UI->>User: Display validation feedback
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
app/src/test/java/com/lxmf/messenger/viewmodel/RNodeConfigValidatorTest.kt
Outdated
Show resolved
Hide resolved
Additional Comments (1)
With Prompt To Fix With AIThis is a comment left during a code review.
Path: app/src/test/java/com/lxmf/messenger/viewmodel/RNodeConfigValidatorTest.kt
Line: 194:194
Comment:
test new minimum boundary SF5 instead of SF7
With `MIN_SF` changed to 5, this should test SF5 as the minimum boundary, not SF7.
```suggestion
assertTrue(RNodeConfigValidator.validateSpreadingFactor("5").isValid)
```
How can I resolve this? If you propose a fix, please make it concise. |
…datorTest.kt with suggestion from greptile-apps bot Fix the error message in the RNodeConfigValidatorTest to SF5 instead of SF7 (Suggestion from greptile-apps) Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…dary check for Spreading Factors
Ok I made the two suggested fixes from the greptile-apps bot. Should I squash the three commits into one or is it fine like this? |
|
And where in the code is the filter to not try to do SF5 on SX127x chipset based RNodes? |
If I'm not mistaken I think that logic is on the RNode firmware, at least that's how it rnsd seems to do it. |
|
Sort of. Its just setting SF6 if you set SF5 on the SX127x The same logic is in the SX126x where its setting SF5 if you set for example SF4 This could confuse people who just try to set things and do not know why its working on the one and not on the other device. The target for Columba are still the users without deep technical knowledge. But in general i would say, it is a benefit to have the ability to do something. Maybe at some later change there should be something to explain the users why things are working as they do. Here you can take a look at the same SF7 limit discussion in other Reticulum projects: markqvist/Reticulum#1036 |
|
Yeah, I mean in our mesh all of the nodes are SX1262 or similar, and for the same reason we are running a custom Meshtastic firmware for the Meshtastic mesh. The possibility should be there, and someone inputing custom values should probably know if their hardware supports that, just like for example you could input 433MHz on a 868MHz RNode and that wouldn´t be a valid config. But how do you check for that? |
|
And do not forget the SX127x handle SF6 different then newer chips. Quote: Source: |
no need to squash. thank you! always take the greptile comments with a grain of salt :) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Hi!
This PR changes the UI check in RNodeConfigValidator and its test on RNodeConfigValidatorTest to allow using Spreading Factors 5 and 6 too instead of just 7-12.
Both the Reticulum RNSD as well as RNode work perfectly with these SFs and we've already tried them both with Meshtastic and RNodes and they work without issues with many different hardwares, so IMHO they shouldn't be disabled.
Also we're looking into using SF6 in our Reticulum LoRa Mesh and that would allow Columba to be used with RNodes on this mesh too.
Also thanks for your great work on Columba!🙂