Implemented YMODEM for embedded target #1
Conversation
removed std and alloc dependencies from ymodem
|
Thank you, but there's too many changes in this PR unrelated to the PR title. Also, did you mean to close the PR? |
|
I would rather have smaller, composable PRs addressing each change, with commits describing the change. |
Cargo.toml
Outdated
| repository = "https://github.com/Cosmo-CoDiOS/txmodems" | ||
| repository = "https://github.com/inApril-AS/txmodems" |
There was a problem hiding this comment.
I would rather this wasn't changed. It looks like you want to rebrand my crate as your own, which isn't fair to my work, and the derived code from other crates.
Cargo.toml
Outdated
| keywords = ["data-transfer", "MODEM", "file-transfer", "embedded"] | ||
| version = "0.1.3" | ||
| authors = ["Dom Rodriguez <[email protected]>"] | ||
| keywords = ["serial", "xmodem", "ymodem", "data-transfer", "MODEM", "file-transfer", "embedded"] | ||
| version = "0.2.0" | ||
| authors = [ | ||
| "Dom Rodriguez <[email protected]>", | ||
| "Eliud de León <[email protected]>", | ||
| "Eskild Jonatan Krumsvik Aanensen <[email protected]>", | ||
| ] |
There was a problem hiding this comment.
Version bump is fine, and keywords are fine - but you should be asking before adding yourselves to the authors key. I was going to put 'The Cosmo-CoDiOS Developers' at some point, given this project is under that umbrella.
| license = "MIT" | ||
| edition = "2021" | ||
|
|
||
| [package.metadata.docs.rs] | ||
| all-features = true | ||
|
|
||
| [features] | ||
| default = [] |
| core2 = { version = "0.4.0", default-features = false } | ||
| crc16 = "0.4.0" | ||
| defmt = { version = "0.3", optional = true } | ||
| heapless = "0.8.0" |
| core2 = { version = "0.4.0", default-features = false } | ||
| crc16 = "0.4.0" | ||
| defmt = { version = "0.3", optional = true } |
There was a problem hiding this comment.
Ah, good catch for logging on embedded.
src/variants/api/ymodem.rs
Outdated
| const SOH: u8 = 0x01; | ||
| const STX: u8 = 0x02; | ||
| const EOT: u8 = 0x04; | ||
| const ACK: u8 = 0x06; | ||
| const NAK: u8 = 0x15; | ||
| const CAN: u8 = 0x18; | ||
| const CRC: u8 = 0x43; |
There was a problem hiding this comment.
Why not use crate::ymodem::Consts? That was written specifically to avoid single-line consts. Please adjust.
| file_name: String, | ||
| file_name: String<32>, |
| extern crate alloc; | ||
|
|
||
| mod common; | ||
| pub mod common; | ||
| pub mod variants; |
There was a problem hiding this comment.
common should remain private!
| pub max_errors: u32, | ||
|
|
||
| /// The number of *initial errors* that can occur before the communication is | ||
| /// considered a failure. Errors include unexpected bytes and timeouts waiting for bytes. | ||
| pub max_initial_errors: u32, |
There was a problem hiding this comment.
Please refrain from unnecessary newlines.
| if self.errors >= self.max_errors { | ||
| #[cfg(defmt)] | ||
| error!("Exhausted max retries ({}) while sending start frame in YMODEM transfer", self.max_errors); | ||
| return Err(ModemError::ExhaustedRetries { errors: self.max_errors }); | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This branch can be simplified. Remove else and simply end function with Ok(()).
remove alloc
Yes, I didn't mean to open it. |
removed std and alloc dependencies from ymodem