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

Add support for E220-900T22S(JP) Lora module #535

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

akif999
Copy link
Contributor

@akif999 akif999 commented Mar 22, 2023

I added driver for E220-900T22S(JP) Lora module (UART interface).

@akif999
Copy link
Contributor Author

akif999 commented Mar 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
Copy link
Member

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
Copy link
Contributor Author

akif999 commented Apr 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
Copy link
Member

Sounds good @akif999 please let everyone here know how we can help!

Copy link
Contributor

@soypat soypat left a 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

Comment on lines +24 to +39
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)
Copy link
Contributor

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.

Comment on lines +24 to +31
TargetUARTBaud1200kbps = 1200
TargetUARTBaud2400kbps = 2400
TargetUARTBaud4800kbps = 4800
TargetUARTBaud9600kbps = 9600
TargetUARTBaud19200kbps = 19200
TargetUARTBaud38400kbps = 38400
TargetUARTBaud57600kbps = 57600
TargetUARTBaud115200kbps = 115200
Copy link
Contributor

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 say BaudRate1200 is equal to 1200. MAybe this should be a Baud type instead and these be reduced to Baud1200.

Comment on lines +110 to +125
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be reduced to d.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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Validate should return an error type itself. check out the errors.Join(errSlice...) function which does pretty much what you are doing below.

@iamemilio iamemilio mentioned this pull request Sep 29, 2023
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