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

Adding new Plugin SIO Reaction #1277

Merged
merged 2 commits into from
Feb 12, 2024
Merged

Adding new Plugin SIO Reaction #1277

merged 2 commits into from
Feb 12, 2024

Conversation

jcassel
Copy link
Contributor

@jcassel jcassel commented Feb 11, 2024

Adding new Plugin SIO Reaction (Sub-Plugin for SIO Control) to the repository.

Adds functionality to the SIO Control plugin enabling a user to configure IO Reactions (Actions to be taken when IO states are changed or to control IO when specific GCode is detected. )

Copy link
Contributor

@jneilliii jneilliii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the couple of comments on this PR, I think maybe the hook_gcode_queuing function should be reworked a little. As it stands now you are processing every command that passes through and then looping through your list of reactions. It would be preferred to exit as early as possibly without doing this. Maybe something like:

if gcode not in [r.Commands[0] for r in self.Reactions if r.RType == SIOReactionType.SIOReactionType.GCODE]:
    return

_plugins/SIOReaction.md Show resolved Hide resolved
authors:
- jcassel
license: AGPLv3
date: 2024-04-11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix date

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the date in the last commit.

_plugins/SIOReaction.md Show resolved Hide resolved
@jcassel
Copy link
Contributor Author

jcassel commented Feb 11, 2024

In relation to the comment about the hook_gcode_queuing,
With much respect, I understand what you want to accomplish by your comment and you make a good point that there are likely ways I could reduce time(iterations) spent in the lookup. But you are suggesting that there is less iteration happening in the structure you provided:

if gcode not in [r.Commands[0] for r in self.Reactions if r.RType == SIOReactionType.SIOReactionType.GCODE]:
    return

I am not sure that is the case.

The for loop still happens over the same list of self.Reaction. It is just hidden in a run time sleight of hand. It maybe even less efficient than a standard looping structure like I have. It looks like its faster because you can do it in one line of code. I think that is about all you get from that change.

In fact the more I look at the structure and what the run time must do, I think it's going to be slower.

It's going to first find the Reactions with Type GCODE and then put that in a list. Then it will loop over that list and check to see if the command[0] in that list is a match. So that is 2 iterations. 1 over the entire list and a second over just the list where RType is GCODE. I am only iterating over the list once. Best case is that it does the check while doing the look up and that is exactly what I am doing.

Additionally if I put what you have suggested at the top of the method, if there was a match, it would cause the need to do an additional full interaction to workout which one or more was the match. Adding a lot of time to the method.

I am open to being convinced otherwise. But I think the best result from the suggested structure is the same as my structure and that is when there are no Reactions that have a GCode match. I would also speculate that what I have is more readable and maintainable.

You did give me an idea where I could front load that lookup at save. I really only need to iterate over the reactions that are of Gcode type. That would truly reduce it to the minimal iterations needed. I will look in to that for a future update.

If you feel that I need to make this change before you would approve the MR, please let me know.

@jneilliii
Copy link
Contributor

You did give me an idea where I could front load that lookup at save. I really only need to iterate over the reactions that are of Gcode type. That would truly reduce it to the minimal iterations needed. I will look in to that for a future update.

Yeah, this would be how I would implement actually. In your on save update an internal list and if the gcode isn't in that list then don't do anything, give back to the process as soon as possible.

The point is, you only need to process the commands when they are configured to be processed. There's no need to loop through your reactions at all if they aren't. I'm sure this is minimal processing even in your for loop because there's probably not that many reactions configured but it's something that would need to be addressed prior to merging please.

@jcassel
Copy link
Contributor Author

jcassel commented Feb 11, 2024

@jneilliii
I am just about to wrap up the changes in the PlugIn itself but noted that I must have the syntax incorrect for the
"plugin_requires" that you suggested.

Should it be like this:
plugin_requires = ["SIO-Control@https://github.com/jcassel/OctoPrint-Siocontrol/archive/main.zip"]

Note that I added the quotes around the string. I did a little searching in the Dev Documentation but did not find anything.

@cp2004
Copy link
Member

cp2004 commented Feb 11, 2024

It's definitely possible to optimise the current implementation, but the list comprehension is not likely to be any quicker than the current implementation, possibly even slower - as x in [list] is slow because it is iterative, and to get the list you have to implement exactly the same for loop from before. I agree with @jcassel's idea.

To eliminate as much of the iterations as possible, since the reactions don't change regularly it should be possible to cache the list, then run the comparison on that.

@jneilliii
Copy link
Contributor

@jcassel try it with space inbetween. I'm doing it this way in bgcode plugin....

https://github.com/jneilliii/OctoPrint-BGCode/blob/c1e45b27a5b685e6ec43f4b3162f5b76f9ab06b1/setup.py#L36

@jcassel
Copy link
Contributor Author

jcassel commented Feb 11, 2024

OK, I think I have got everything in place.
I corrected and tested plugin_requires. (all good) installs SIOControl when not installed.

I updated the SIOReaction plugin to use a specific list for Gcode Reaction types. It will pop out of the method if there are none and since it is a list only of the GCode implementations, it will only check to match where it makes sense.

@jneilliii jneilliii merged commit 45b11f9 into OctoPrint:gh-pages Feb 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants