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

Unified upload and run logic #193

Merged
merged 2 commits into from Oct 7, 2019
Merged

Unified upload and run logic #193

merged 2 commits into from Oct 7, 2019

Conversation

@ngwese
Copy link
Member

@ngwese ngwese commented Oct 2, 2019

This is a work in progress intended for discussion as to approach. If this is a reasonable direction to go I will do further testing and cleanup.

The goal is to have consistent upload limitations, parse/eval, and vm reset behavior across the druid upload (aka flash) and run operations.

The approach is to have both upload and run perform bulk uploads by issuing a ^^s (start) command, sending the script, and then signaling completion with either a new command; ^^f (flash), in the upload case or the existing command; ^^e (end) in the run case.

A cursory amount of testing (with a modified druid) appears to confirm the basic approach works. Items worth noting:

  • The notion of the repl_mode is now entirely internal to the repl code.
  • Lua_Reset() is called to free resources ahead of upload buffer allocation because repeated attempts to run code seemed to fail (likely in allocation) and leave the device in an inconsistent state (previously druid would issue a ^^k to reset ahead of an upload)
  • Previously upload buffer allocation failures weren't really being handled. Now when allocation fails the repl is put into REPL_discard mode which simply throws away everything received until such time as either of the ^^f or ^^e stop commands are received.
  • Previously upload was not checking to see if the 8kB upload buffer size was exceeded. Now if the upload exceeds 8kB (USER_SCRIPT_SIZE) the buffer is freed and the repl is put into REPL_discard mode.
@ngwese ngwese added the do not merge label Oct 2, 2019
@ngwese ngwese requested review from trentgill and tehn Oct 2, 2019
@ngwese
Copy link
Member Author

@ngwese ngwese commented Oct 2, 2019

One additional thought is that this change does redefine the behavior of ^^e so that is a breaking change w.r.t. druid and most like the max/m4l objects.

@ngwese
Copy link
Member Author

@ngwese ngwese commented Oct 2, 2019

For reference the druid changes look like this: ngwese/druid-1@b9a30f6

@tehn
Copy link
Member

@tehn tehn commented Oct 2, 2019

these changes sound good! I can help test perhaps late tonight.

@trentgill
Copy link
Collaborator

@trentgill trentgill commented Oct 2, 2019

@ngwese
Copy link
Member Author

@ngwese ngwese commented Oct 2, 2019

I was curious about memory fragmentation as well. My initial approach was to at least start by trying to work within the existing buffer setup and stick to the existing allocation patterns.

@tehn
Copy link
Member

@tehn tehn commented Oct 6, 2019

perhaps this is the solution for the mac os druid bugs as well?

ngwese added 2 commits Oct 2, 2019
- discard upload progress if buffer allocation fails
- discard upload if USER_SCRIPT_SIZE is exceeded
@ngwese ngwese force-pushed the ngwese:unified-upload branch from d06acdc to c5eb245 Oct 6, 2019
@ngwese
Copy link
Member Author

@ngwese ngwese commented Oct 6, 2019

@tehn - this does not appear to fix the mac druid issues but they might be reduced (just based on my usage of late)?

i've redone the commit messages and so far this has been working well for me.

@ngwese ngwese added enhancement and removed do not merge labels Oct 6, 2019
@trentgill
Copy link
Collaborator

@trentgill trentgill commented Oct 6, 2019

(aside: i think the druid issues are to do with timing in the python script)

hoping to get this merged tonight after i can test it a bit more.

@trentgill
Copy link
Collaborator

@trentgill trentgill commented Oct 7, 2019

Merged this as it seems to be working ok on my setup. i'm going to add a couple helpers & change the init() fn calls & make a PR for druid to capture these things & remove unnecessary ^^k etc commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants