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

model01: Use a modified CDC.cpp #7

Merged
merged 2 commits into from Dec 12, 2018
Merged

model01: Use a modified CDC.cpp #7

merged 2 commits into from Dec 12, 2018

Conversation

algernon
Copy link
Contributor

At least for OSX, we need to slow down Serial writes. The easiest way to do that is to add a delay after each write, and to accomplish that, the easiest is to modify CDC.cpp. Without this, on OSX, we end up writing too fast (and changing the baud rate does not help, no matter in which direction I change), and end up losing data.

This is not a great solution. It's not even a solution, but a workaround. But it works for now.

@algernon algernon added the bug Something isn't working label Oct 26, 2018
@algernon algernon requested a review from obra October 26, 2018 16:25
@obra
Copy link
Member

obra commented Oct 26, 2018

Can you recast this commit as checking in the original file and then checking in your change as a separate commit?

@algernon
Copy link
Contributor Author

Sure, will do that in a bit.

We're going to change this file, so add the original (from arduino-1.8.7,
hardware/arduino/avr/cores/arduino/CDC.cpp) before applying our changes.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
At least for OSX, we need to slow down Serial writes. The easiest way to do that
is to add a delay after each write, and to accomplish that, the easiest is to
modify `CDC.cpp`. Without this, on OSX, we end up writing  too fast (and
changing the baud rate does not help, no matter in which direction I change),
and end up losing data.

This is not a great solution. It's not even a solution, but a workaround. But it
works for now.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
@algernon
Copy link
Contributor Author

Updated

@algernon
Copy link
Contributor Author

Having made all communication go through Focus (keyboardio/Kaleidoscope#477), I made an attempt at adding the delay to Focus.send(). That... doesn't scale well, because .send() is inlined. Thus we end up with not calls to Focus.send(), but calls to Serial.print and delay. With my own sketch, this is over 200 bytes extra, and each call to Focus.send() will adds yet another delay call.

I could redo keyboardio/Kaleidoscope#477 in a way that .send does not get inlined, but that will also add significant PROGMEM costs (though less than 200 bytes, but still too much), would use more memory, and would be a little bit slower too.

I'm afraid that unless we figure out the root cause, and can fix that, this delay in CDC is our most efficient bet. :(

@obra
Copy link
Member

obra commented Nov 23, 2018 via email

@algernon
Copy link
Contributor Author

Didn't try yet, will do so later tonight.

@algernon
Copy link
Contributor Author

Forced noinline for all send methods: +392 PROGMEM with my Atreus sketch, +684 with my Model01. Forcing no-inline only on the non-templated ones results in +30 PROGMEM only, which would be manageable. Adding delay(5) to the mix results in +254 PROGMEM.

This PR is a constant +28 bytes. Seems like we exhausted most other options... :/

@obra
Copy link
Member

obra commented Dec 12, 2018

This makes me really unhappy, especially since it only works for the Model 01 and isn't a general solution. But I'm ok with it being merged for now.

I still don't like it.

@algernon
Copy link
Contributor Author

We'll figure out something better before 2.0. I... have a couple of ideas, but it will take a while to explore them, and I'd rather not hold up Chrysalis usability on OSX until then.

@algernon algernon merged commit 029a563 into master Dec 12, 2018
@algernon algernon deleted the osx/cdc-slowing-down branch December 12, 2018 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants