Micropython/patch drbitboy 20221231

[drbitboy]

Short description of change

Change [u]pylogix/lgx_uvendors.[m]py file lookup to use binary search

Types of changes

New code in pylogix/lgx_uvendors.py New fixed-length-record file upylogix/vendors.bin

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] I have read the docs/CONTRIBUTING.md document.
• [ ] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have read tests/README.md.
• [ ] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

What is the change?

Change [u]pylogix/lgx_uvendors.[m]py file lookup to use binary search

What does it fix/add?

Faster lookup Old O(N) code replaced to ensure old assignment of line.split(':') result does not cause an error

Test Configuration

• PLC Model
• PLC Firmware
• pylogix version
• python version
• OS type and version

[drbitboy]

A few things left to resolve before considering this PR for merging:

• Should we remove the asserts in getitem_O_logN?
• Are there ways to speed up the vendorID comparisons? E.g. locate colon in lread string variable, do a string compare, only read first few characters of each line?
• Should we remove the character from vendorID 721:Industrial Control\tCommunication, Inc.? This is something that should probably be done to the master branch anyway.

[drbitboy]

Update: I did a test of removing the asserts; they don't have a noticeable affect on performance.

[drbitboy]

Premature Optimization, Update II: doing vendorID checks via string comparison speeds up code by about a third.

N.B. these results are on a desktop running WSL2, so the ARM results would be different in absolute magnitude, but probably similar in relative magnitude.

N.B. a similar approach might also speed up the linear search

$ ./build_mpy.sh
[]

real    0m0.019s
user    0m0.019s
sys     0m0.000s
[]

real    0m11.705s
user    0m7.207s
sys     0m4.498s
[]

real    0m0.102s
user    0m0.081s
sys     0m0.020s
[]

real    0m0.103s
user    0m0.062s
sys     0m0.041s```

[drbitboy]

Ha, one more: vendorID 248 has a space after the colon!

[TheFern2]

Nice improvement on the look up optimization. Yes, we should remove extra \t and spaces in strings, spaces if they're leading or trailing.

A few comments:

Let's remove the python code from build_mpy.sh, mingling python in sh makes it harder for autocomplete and linting, (I've dealt with vim plugins where vimscript and python are mingled and is painful). Let's make a build_vendor_bin.py and call that script from the sh script.

Let's add some vendor lookup unittests both pass and fail tests. We should probably add another file for testing these internal classes. Later on I will add some regex unittests as well on the same file something like UtilsTesting.py?. Can we rename VENDORS class to Vendors. All upper case feel more like a constant.

Btw you could have kept working on your original micropython/patch1 branch, you just go to dmroeder/pylogix repo pull requests page and when you click on make pull request you select the branches yourself. It just won't pop up like in the beginning because of the first merge if I'm not mistaken.

[drbitboy]

okay, I will do all of that. I also made some more changes (faster key check, removed asserts, etc.)

I made a separate branch for the second PR because that's what I thought @TheFern2 meant by "...Once you make a PR and is merged is a done deal for that PR...." in the discord. Whatever ...

[drbitboy]

Let's remove the python code from build_mpy.sh, mingling python in sh makes it harder for autocomplete and linting, (I've dealt with vim plugins where vimscript and python are mingled and is painful). Let's make a build_vendor_bin.py and call that script from the sh script.

On this, in which directory of the repo do you want to put build_vendor_bin.py? IMO putting it in the top level adds clutter, which is why I put it inside the shell script (self-contained => +1). It's a short convenience script for build and not part of the app itself, but I'm okay with separating it out.

[drbitboy]

Everything except unit tests complete.

At some point we should probably drop vendors.txt.

[drbitboy]

This should be ready, or close anyway.

[TheFern2]

We could move scripts (sh, py) into a scripts directory in the root dir to make things cleaner. If vendors.txt is no longer needed for micropython we can remove it.

In regards to a "PR is done once is merged", what I meant was no further commits are pushed from your branch to the upstream branch. But your branch can still open new PRs if that is makes sense. But is all good no worries.

[TheFern2]

Is this PR is ready to be reviewed and merged? I'll prob get to it sometime this week(end).

[drbitboy]

Is this PR is ready to be reviewed and merged? I'll prob get to it sometime this week(end).

yes with the following caveats 1) unless we want to move those build files into a build_scripts/ sub-directory, and maybe add a Makefile 2) I only have a Micro820 and no CLogix stuff, so most of the tests are skipped. 2.1) Can [Emulate 5000] v16 be used for the C*Logix testing?

[TheFern2]

1. unless we want to move those build* files into a build_scripts/ sub-directory, and maybe add a Makefile

Sounds good, in the beginning I wanted to leave everything in root, but maybe in the future we add more scripts, might as well make the scripts dir to keep root dir clean.

2.1) Can [Emulate 5000] v16 be used for the CLogix testing?

Yes, emulate 5000 should suffice if I am not mistaken. Only thing is that you need to run everything locally, if you have it on the same machine then you should be ok but if is on a VM you'll need to run everything inside the vm.

https://github.com/dmroeder/pylogix/issues/23 Emulate discussion a few years back

[drbitboy]

Hmm, I only have Emulate 5000 on an XP laptop, not a VM.

Options:

• Put Python onto that XP laptop, which is already creaky enough?
• Use PuTTY to forward some ports from the XP laptop's localhost to WSL2 on another maching?
• Cygwin?

[drbitboy]

How do you feel about a Makefile to run build* scripts?

On second thought, those build scripts only apply to micropython, so maybe a micropython/ sub-directory (or even the existing upylogix/). That's a special-purpose use case, so it could have it's own README, and we could add a reference to micropython/ once now in the root README and never need to touch that root README again, at least not for micropython stuff.

[TheFern2]

I think a general scripts directory should suffice, as long as micropython scripts have mpy it should be a clear indication that is for micropython.

When you say Makefile, what do you have in mind? I think the original build_mpy.sh is enough, what benefits or advantage does a Makefile give us over build_mpy.sh? I usually don't associate Makefiles with python so I am a little bit out of the loop on python Makefiles.

[TheFern2]

Hmm, I only have Emulate 5000 on an XP laptop, not a VM.

Options:

* Put Python onto that XP laptop, which is already creaky enough?

* Use PuTTY to forward some ports from the XP laptop's localhost to WSL2 on another maching?

* Cygwin?

I can test the clx stuff, I have a setup ready for that.

[drbitboy]

the make command is one conventional way to build "everything" and/or sub-systems. It's basically a way to execute one or several commands with built-in option parsing.

E.g. the command could be make micropython to build micropython-specific files (e.g. run those new build* scripts).

At this point, just the make command by itself could build all of the micropython files, without getting in the way of regular Python installs (in case someone ran it by reflex), because regular Python needs nothing at this point.

To me, it's better than adding "blah blay type './build_mpy.sh' blah blah" in the readme, because you would need to type the ./ prefix to do that, and to know the specific filename.

[TheFern2]

How easy is to install and use make on plain windows, and windows wsl? If the answer is intermediate to difficult, I'd say to stay away from make. I think the more we keep everything OS generic and python based the better. Otherwise it makes everything harder for contributors in the future. I was actually thinking of making build_mpy.sh a python script since mpy-cross is a python package. Initially I made it a shell script just to get things going in the beginning because I am on linux but I knew we need portability later on.

Do we really, really need to add another level of complexity for these simple build scripts? I feel like this whole vendor thing has gotten way out of proportion and we still yet don't know if Discover will even work. We have done about 5-7 optimizations for something we don't know will work.

@dmroeder your thoughts on a make build system?

[drbitboy]

build_mpy.sh uses bash; that is as problematic for windows, more or less, as make.

Make is built-in to a typical WSL environment.

I agree that converting all build actions to python statements probably makes the most sense.

[TheFern2]

Yeah I made the sh script in a jiffy but in the back of my mind I knew we needed portability. When I made the script I was thinking of maybe adding either a bat file or py.

WSL isn't typical for most plc folks specially if you run vmware or vbox. Typically you have a vm for x versions of rockwell. If you enable wsl you can't run vbox and viceversa. So while wsl is nice, it is bad to assume that everyone on windows will run wsl.

Let's keep portability with python scripts to make things easier for everyone.

[drbitboy]

Agreed, hold off on the merge until we fix up the scripts.

I won't get to it tonight. ... I did get to it tonight.

[drbitboy]

Do we want a README in scripts/?

[TheFern2]

We can, more documentation is always nicer.

[drbitboy]

Okay, but I won't get to it today ;)

[TheFern2]

Going to process this PR tomorrow, been a busy week at work. Is ok for now without the readme. That can be a separate PR if you want.