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. |
shymega
left a comment
shymega
left a comment
There was a problem hiding this comment.
Several changes needed, and if you can open other PRs to address this, that would be great, and we can close this one as the parent PR.
Cargo.toml
Outdated
| name = "txmodems" | ||
| readme = "README.md" | ||
| 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.
| keywords = ["data-transfer", "MODEM", "file-transfer", "embedded"] | ||
| version = "0.1.3" | ||
| authors = ["Dom Rodriguez <shymega@shymega.org.uk>"] | ||
| keywords = ["serial", "xmodem", "ymodem", "data-transfer", "MODEM", "file-transfer", "embedded"] | ||
| version = "0.2.0" | ||
| authors = [ | ||
| "Dom Rodriguez <shymega@shymega.org.uk>", | ||
| "Eliud de León <eliud.deleon.10@gmail.com>", | ||
| "Eskild Jonatan Krumsvik Aanensen <eskild.krumsvik.aanensen@inapril.com>", | ||
| ] |
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.
| core2 = { version = "0.4.0", default-features = false } | ||
| crc16 = "0.4.0" | ||
| defmt = { version = "0.3", optional = true } | ||
| heapless = "0.8.0" |
| file_name: String, | ||
| file_name: String<32>, |
|
|
||
| extern crate alloc; | ||
|
|
||
| mod common; | ||
| pub mod common; | ||
| pub mod variants; |
| 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. |
|
Hi,
Why was this PR closed?
I would like to have your changes upstreamed - I saw briefly on your
fork that you have implemented `embedded-io-async` support, and this is
very promising.
Whilst I can't dictate that you upstream the changes, as the license
isn't copyleft, I would be very open to accepting your changes, and
working on a fully open-source implementation.
On 13.11.2024 01:52, inApril-AS wrote:
Closed #1.
--
Reply to this email directly or view it on GitHub:
#1 (comment)
You are receiving this because you were assigned.
Message ID: ***@***.***>
Best wishes,
--
Dom Rodriguez
|
|
Hi, The PR was automatically closed since I made the repository private. Eskild J. K. Aanensen |
|
On 14.11.2024 01:56, Eskild Jonatan Krumsvik Aanensen wrote:
The PR was automatically closed since I made the repository private.
I did this because I thought the changes strayed to far from the original crate, and I was unsure of my employers take on it being open source.
But since you are interested, and I got a yes from my employer I can reopen new issues on a new fork.
That's excellent to hear! Please do feel free to open a new fork & PR. I
will then do a review, and try and get it merged in.
I'm going to setup a CI workflow and automatic Dependabot merges as well
at some point.
Do you think it's worth adding CI tests for mocking the YMODEM/XMODEM
implementation streams?
Thank you for your efforts on this front.
Best wishes,
--
Dom Rodriguez
|
|
I have opened a new fork, and will do a PR soon. As I have gotten a prototype working receiving data. I don't know if adding CI is worth it, I think that would be up to you. Eskild J. K. Aanensen |
|
Hi, I've reviewed #2. As for CI, I was not thinking necessarily of testing real-world data, but just mock tests of 'example' data. We could use the base Modem trait for that. |
|
Feel free to open a PR adding YMODEM support, and your |
3 participants
removed std and alloc dependencies from ymodem