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

Proposed fix for issue #46 #47

Merged
merged 19 commits into from
Mar 5, 2017
Merged

Proposed fix for issue #46 #47

merged 19 commits into from
Mar 5, 2017

Conversation

int3ll3ct
Copy link
Collaborator

The way the freshroastsr700 package is designed, there is the ability to speicfy a callback function associated with the freshroastsr700's time_remaining countdown reaching zero. This callback is called from a child process spawned by freshroastsr700.

This has a number of significant design implications for users of freshroastsr700, the major one being, the callback functions you supply will be called from a different process. To do this, Python will make a copy of whatever object is the parent of the callback, at the time the child process is launched. The state of your object's state variables in the callback context are therefore the state they were at at process launch time. These callback functions can only correctly communicate back to the rest of your application via process-safe means. Anything your freshroastsr700 callbacks touch need to be designed to do IPC.

In Openroast, openroast.roaster (a freshroastsr700 instance) sets up callbacks into a Recipe object. These callbacks do not produce the desired behavior because Recipe is not designed for multiprocessing.

To fix issue #46, the Recipe object had to be completely re-factored to store all its information in shared memory, thereby allowing both Openroast and the freshroastsr700's child processes to properly access the Recipe state throughout a roast.

In addition to this, openroast.roaster calls back into openroast.recipe which, in that callback, also then attempted to call back into the RoastTab itself. For similar reasons, this does not work, and to solve this problem, a simple process-safe flag is now set up as the sole communication means from openroast.roaster back into RoastTab. This flag causes an asynchronous refresh of the new recipe step's fan and targetTemp info.

This makes Openroast fully functional (for the base use case...) on Ubuntu 14.04. Tested with Python 3.4 on a Dell XPS 9343.

It would be important to verify that this pull request does not break functionality in the Windows and Mac versions. I am not set up to test that.

Alan@int3ll3ct added 3 commits February 7, 2017 22:20
…st could not automatically moving to the next roasting step in the recipe because the Recipe object is accessed from multiple processes, and the data in the object needed to reside in shared memory.
@int3ll3ct
Copy link
Collaborator Author

Well, travis build is failing, but I don't see where PyQt5 is installed as part of running the tests, so not sure how to move past this hurdle right now.

Alan@int3ll3ct added 15 commits February 9, 2017 22:23
…esting. Take 10, I wish it was easier to test this offline...
@int3ll3ct
Copy link
Collaborator Author

I give up. Getting PyQt5 properly installed on Travis CI is a complete nightmare. Not sure where to go from here.

@int3ll3ct
Copy link
Collaborator Author

I've looked at the TravisCI whitelisted packages for trusty (14.04), and most Qt5-related stuff is not there. python3-qt5 certainly isn't. This is why this travis setup will not work without a great deal of effort.

@betterengineering
Copy link
Contributor

Thanks for putting this together! The unittests for this application are rather underdeveloped (roasting pun intended). It looks like they are only testing instantiation and are not actually running any tests. We can do one of two things. We can remove the tests that require pyqt, since we will definitely know if the main window has been created or not 😊. Or we can remove Travis integration since the tests are not really serving their purpose anyways.

@int3ll3ct
Copy link
Collaborator Author

I suggest we remove the Travis integration, as the result is not CI - it doesn't spit out an installable application nor a deployment, which is what I'd expect out of Travis/Jenkins etc. Since TravisCI can't build PyQt5 apps, based on my research, I suggest we add build scripts for Windows and Mac to the project so that there is a repeatable way to generate installable versions of the app. For Linux, IMO it's sufficient to ask someone to simply perform a development installation (although we could eventually add a .deb packaging script at some point).

I'll add yet another commit that removes the travis file. I'll put in a separate feature request for the build script. I want to turn my attention back to freshroastsr700 for a bit.

… package (it's not a whitelisted package in the Travis CI system).
@CCoffie CCoffie merged commit 6a16a56 into Roastero:master Mar 5, 2017
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