- Notifications
You must be signed in to change notification settings - Fork230
Add support for E220-900T22S(JP) Lora module#535
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:dev
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
akif999 commentedMar 22, 2023
@sago35 Can you do a review? The result of the response to the indication that you have not yet confirmed is described below. |
deadprogram commentedMar 28, 2023
Hello@akif999 if you implement this interface, then this device should be able to work with the LoRaWAN package just like the other LoRa radios:https://github.com/tinygo-org/drivers/blob/dev/lora/radio.go What do you think? |
akif999 commentedApr 11, 2023
Hello@deadprogram I get it. I think it is desirable to follow the interface you presented. So I will update my implementation. I'm thinking of updating the implementation and moving forward with this pull request. |
deadprogram commentedApr 11, 2023
Sounds good@akif999 please let everyone here know how we can help! |
soypat left a comment
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.
quick drive-by comments
| bytes *[]byte, | ||
| ) error { | ||
| // byte [8] is read only | ||
| if len(*bytes) < E220RegisterLength-1 { | ||
| return fmt.Errorf("length of bytes must be greater than or equal to %d: got=%d", E220RegisterLength-1, (*bytes)) | ||
| } | ||
| (*bytes)[0] = byte((c.ModuleAddr & 0xFF00) >> 8) | ||
| (*bytes)[1] = byte((c.ModuleAddr & 0x00FF) >> 0) | ||
| (*bytes)[2] = byte(((c.UartSerialPortRate & 0x07) << 5) | (c.AirDataRate & 0x1F)) | ||
| reserved := byte(0b000) | ||
| (*bytes)[3] = byte(((c.SubPacket & 0x03) << 6) | ((c.RssiAmbient & 0x01) << 5) | ((reserved & 0x07) << 2) | (c.TransmitPower & 0x03)) | ||
| (*bytes)[4] = byte(c.Channel) | ||
| reserved = byte(0b000) | ||
| (*bytes)[5] = byte(((c.RssiByte & 0x01) << 7) | ((c.TransmitMethod & 0x01) << 6) | ((reserved & 0x07) << 3) | (c.WorCycleSetting & 0x07)) | ||
| (*bytes)[6] = byte((c.EncryptionKey & 0xFF00) >> 8) | ||
| (*bytes)[7] = byte((c.EncryptionKey & 0x00FF) >> 0) |
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.
No need to pass in a pointer to a slice. A slice is already composed of a pointer to the underlying array. So you can change(*bytes)[0] = byte(...) to ``bytes[0] = byte(...) andparamsToBytes(bytes *[]byte)` to `paramsToBytes(bytes []byte)` and you'll get same behaviour.
Also one tends not to break function signatures with newlines in Go, especially if its a sufficiently small function signature.
| TargetUARTBaud1200kbps = 1200 | ||
| TargetUARTBaud2400kbps = 2400 | ||
| TargetUARTBaud4800kbps = 4800 | ||
| TargetUARTBaud9600kbps = 9600 | ||
| TargetUARTBaud19200kbps = 19200 | ||
| TargetUARTBaud38400kbps = 38400 | ||
| TargetUARTBaud57600kbps = 57600 | ||
| TargetUARTBaud115200kbps = 115200 |
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.
Does this add any value? It seems like a round about way to sayBaudRate1200 is equal to1200. MAybe this should be aBaud type instead and these be reduced toBaud1200.
| switch mode { | ||
| case Mode0: | ||
| d.m1.Low() | ||
| d.m0.Low() | ||
| case Mode1: | ||
| d.m1.Low() | ||
| d.m0.High() | ||
| case Mode2: | ||
| d.m1.High() | ||
| d.m0.Low() | ||
| case Mode3: | ||
| d.m1.High() | ||
| d.m0.High() | ||
| default: | ||
| return fmt.Errorf("Invalid mode: %d", mode) | ||
| } |
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.
this could be reduced tod.m0.Set(mode&1 != 0); d.m1.Set((mode>>1)&1 != 0) and a error check if mode is greater than 3
| // WriteConfig writes configuration values to E220 | ||
| func (d *Device) WriteConfig(cfg Config) error { | ||
| cfg.Validate() |
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.
Validate should return anerror type itself. check out theerrors.Join(errSlice...) function which does pretty much what you are doing below.
I added driver for E220-900T22S(JP) Lora module (UART interface).