levylabpitt / AFM-Lithography

BSD 3-Clause "New" or "Revised" License
6 stars 0 forks source link

Refactor into classes/SMOs #16

Open ciozi137 opened 4 years ago

ciozi137 commented 4 years ago

first draft:

Monkeymerlot commented 4 years ago

Do we want start from scratch with a brand new architecture and as we build new parts of the architecture also work them into the old, or just try and work them into the old?

I think (and I might be wrong about this) but the first option gives us the most flexibility while still improving existing software. Then we can have a minimum viable product with the architecture that isnt pieced together, and switch over. I think that we should refrain from adding features to the existing architecture if we are to switch, otherwise we will never end up switching because we will always be playing catchup...

New Architecture

Pros:

Integrating Old Architecture

Pros:

These are just my thoughts on this, and like I mentioned earlier I am heavily biased but I do think that a new architecture would be a lot better in terms of refactoring, especially since we can write tests and develop something intended from the start for use with SMOs and Asynchronous processes, rather than reworked to use them. Plus we wont have to worry about breaking existing software.

ciozi137 commented 4 years ago

@Monkeymerlot food for thought:

https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/

Monkeymerlot commented 4 years ago

I read that years ago and honestly it kept going through my head when writing that! I couldnt find it again. The same kind of idea is also written by Robert Martin (One of the original signatories of the Agile Manifesto) in that this is probably a mistake as well. However, I think that writing a new architecture from scratch is different from starting a program from scratch. I think what the way I want to do is to write the architecture, then use the existing code to populate the architecture. We can then go back and refactor it to be better if we want, but we will then have a working product.

ciozi137 commented 4 years ago

We do have an architecture already though: instrument.lvclass/smo. I’m a fan of a hybrid approach of compartmentalizing the code and converting each compartment to the new architecture. So more or less the ones I outlined above, and converting each one separately to the new architecture will allow us to keep existing code while simultaneously improving it. And I agree with your comment elsewhere that simplifying the existing code will go a long way toward achieving that goal.

On Thu, Jun 18, 2020 at 8:22 PM Joseph notifications@github.com wrote:

I read that years ago and honestly it kept going through my head when writing that! I couldnt find it again. The same thing idea is also written by Robert Martin (One of the original signatories of the Agile Manifesto) in that this is probably a mistake as well. However, I think that writing a new architecture from scratch is different from starting a program from scratch. I think what the way I want to do is to write the architecture, then use the existing code to populate the architecture. We can then go back and refactor it to be better if we want, but we will then have a working product.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/levylabpitt/AFM-Lithography/issues/16#issuecomment-646368869, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGLMUZHFYMHRELMXLKI34JLRXKVURANCNFSM4OCD3IUQ .

Monkeymerlot commented 4 years ago

SMOs are a messaging framework. The instrument.lvclass is additional tools + extras that add onto that framework at least as how I see it. What I mean by architecture is this: image Which would probably have an architecture like this: image I am talking about how the things are decoupled and how messages are handled.

ciozi137 commented 4 years ago

This looks good. This might be tricky to manage, but I'm thinking we can have a "new architecture" branch AND develop Parse, UI, etc submodules simultaneously that can be merged with existing production branch sooner before "new architecture" is ready

Monkeymerlot commented 4 years ago

Adding new features might look like this: image

Monkeymerlot commented 4 years ago

I think that is ideal, or they are developed as separate projects in the master branch?

Monkeymerlot commented 4 years ago

I think that the parsing is a good place to start for classes because there are a lot of similar objects that are just different enough that they can really utilize inheritance and this would make a good exercise of a factory pattern. image https://www.w3.org/TR/SVG11/paths.html#PathDataClosePathCommand

Plus this would make it more scalable, which is something we've had issues with in the past. eg the horizontal and vertical lineto commands are not supported in our program, and when they are input anyways the parser gets stuck in a loop. When we first made the program, Inkscape didn't support them. However, Inkscape now supports them, but we can't use the most recent versions of Inkscape because of it causing issues in our parser.

ciozi137 commented 4 years ago

@Monkeymerlot re: what you showed yesterday, how will that fit into an SMO architecture? Does the AFM_driver need to be converted to an SMO? Or will there be an SMO that calls the driver? (Edit: add reference to #25)

Since the driver part of the existing program is already a process that is called asynchronously I think it is also an ideal place to replace with an SMO (in addition to the parse library that you mentioned)

Monkeymerlot commented 4 years ago

@ciozi137 I designed the AFM_Driver_Manager architecture to allow for a relatively painless conversion into SMOs. The idea is that the driver will be an SMO based on an AFM HAL that uses the Instrument Framework in the future. The AFM_Driver_Manager is a bridge to this conversion basically. We badly needed to change the way that we stop the AFM drivers, and I wanted to switch over to an event based messaging scheme. But to do these things I would probably need a utility class that would manage lifetimes, stopping etc. that will eventually be replaced by the SMO framework itself, as it does that stuff automatically. Furthermore, I also want to add a watchdog to the lithography to provide a true abort sequence, with post abort actions so I wanted to set up a mini architecture that would allow for that addition as well without having to work against the SMO Framework until I got the bugs sorted out.

Monkeymerlot commented 4 years ago

Path Parser just passed all unit tests for the different objects: image Class Hierarchy: image I'll be committing this to the develop branch tonight.

Monkeymerlot commented 4 years ago

The files have been committed to the develop branch. The Parse Testing project is a little messy because I am still planning things out somewhat.

I also started writing tests for a path tokenizer that wont pass yet because no code has been written for the Tokenizer yet. In particular, there are a lot of tests involving formatting (50!) because as I understand the specification, the formatting is rather vague on purpose because SVGs are meant to be really flexible. (ie you can use spaces, commas, or nothing at all in certain positions to delimit the coordinates/commands). And there is nothing that says you have to stick with the same formatting, even in the same command sequence.

Alternatively, I could have designed the system to comply with the formatting of Inkscape itself. I chose not to because I don't think that it will reduce the complexity of the code that much beyond less tests, which I have already written. Furthermore, adhering to the capabilities of Inkscape has caused us issues in the past already. In particular, Horizontal Lineto and Vertical Lineto commands were not supported when this program was first written, but they later became supported. Now if we use a newer version and there happens to be a perfectly straight horizontal or vertical line (fairly common) it gets stuck in a loop.

Another thing that caused a huge number of tests is that I am going to be using lexical classes instead of regular expressions to tokenize the path command into different commands, so that means that relative commands (lower case) and absolute commands (upper case) have the possibility to behave differently. So this nearly tripled the formatting tests (I had to consider mixed cases as well.

Feel free to check it out and let me know what you think. (for the path commands, Im still working on the tokenizer)

Monkeymerlot commented 4 years ago

Tokenizer is finished and passes all of the required tests that I made last night and today. It gets rid of complicated and/or implicit path command data and breaks the entire path command string into chunks that correspond to a single path command and can be used in conjunction with the Path Commands classes that I finished yesterday. It turned out to be a little bit complicated but I guess the main idea is that it walks along the string (which is initialized as an array of individual characters) and figures out what the commands are and how much data is needed along with making implicit commands explicit.

Since it turned out to be fairly complicated, I am going to walk any future people looking at the code through it here...

This next part is a deep dive (fair warning) into the implementation of the Tokenizer: (Moved to Wiki) Path Tokenization

Monkeymerlot commented 4 years ago

Parsing for Paths: (Moved to Developer Wiki)

Inside ParseSVG:

Inside ParseSVG.lvclass:GetPaths.vi: image This is a tad bit messy looking because of the nature parsing xml in LabVIEW.

Inside ParseSVG.lvclass:GetRects.vi: image I had to make this one a little bit more complicated because of the sheer number of attributes that rectangles have. Basically it uses the typedef which is named with the attribute names for its elements, and then uses that to find the values. Afterwards it basically coerces it back into the wanted data type.

ciozi137 commented 4 years ago

@Monkeymerlot this looks like really great documentation, thank you. Do you think it should be moved to its own readme file inside, for example, a documentation folder?

Monkeymerlot commented 4 years ago

@ciozi137 I moved it into the newly created developer documentation section of the wiki.

ciozi137 commented 4 years ago

Nice

Monkeymerlot commented 4 years ago

Implemented a rectangle class and provided a way to parse images image

I was also thinking of using a decorator pattern for the funnel implementation since they are not only identical to paths, but are rather just a few extensions to the same exact data but there is nothing that warrants them being a child class as they are not a physical object that we can parse from the svg document.

Monkeymerlot commented 4 years ago

Finished SVGImage class which will store data for the image that is found, and then I made an SMO called ParseVectorFile Class (we can change this in the future using the Open G LVOOP toolkit) and implemented a method that will return the parsed data in the form of objects when it is given a path: image And a look inside Parse SVG: image Currently there is no checks on the path type etc, but I can add them in later. For now I was more concerned with getting the API for the SMO in line and making sure it worked correctly. I will also break up each type of object into its own method I think to make the diagram a little smaller. Also here is a picture of the test launcher: image

@ciozi137 let me know what you think.

Monkeymerlot commented 4 years ago

Modularized (so multiple people can work on similar things and each part is easier to understand) and added a synchronous method. Synchronous Method: image image where if it is not started it will provide stale data: image

Modularization of Parse SVG: image Get SVG Objects: image Path Factory: image Rectangle Factory: image Image Factory: image

Monkeymerlot commented 3 years ago

Wrote glue code so we can use the new parser in the current software.

The result returned from the ParseVectorFile SMO is then run through the glue code which converts it into the old format. image

And here is the glue code itself: image Which is using the APIs for the parsed data to get the values and turn it into something that the current software can use. In the future, as we convert different parts to the new class based format, we would just need to delete the glue code.

ciozi137 commented 3 years ago

@Monkeymerlot Thanks for the updates. I haven't looked closely yet, but how difficult do you think it might be to add a GDSII parser?

ciozi137 commented 3 years ago

Sorry, my question wasn't very specific. Adding GDSII was always part of the overhaul plan. It looks like you have implemented a "parser" class and svg implementation. If that's the case, How close are we to being able to add GDSII? And how difficult?

Monkeymerlot commented 3 years ago

@ciozi137 I actually have looked into a GDSII parser in pretty close detail. It's not impossible, it is just a lot less forgiving than an SVG parser.

Some key points:

Other than that it wouldn't be that difficult. I literally made the new parsing architecture to deal with the possibility that we might change file types in the future.

ciozi137 commented 3 years ago

Superb. I thought you have the parsing architecture built in but was confirming. We have started to use GDSII with the new EBL (using KLayout). It would be great to consolidate if possible. @aditi-nethwewala can chime in with which objects they are using and maybe even a layout file. I imagine we can implement a small subset of object types to begin with.

Is it possible to use an existing library to parse the GDSII?

Monkeymerlot commented 3 years ago

Some questions and more details: First, I have attached the file where I was exploring GDSII parsing (I had to pull it out of my recycle bin, good thing I never clear it). It is really messy but I didn't want to put too much time into it because at that point it was only a side comment and we didn't have the EBL yet.

And there are no existing libraries for GDSII. I checked this summer and remember being bewildered at the fact that nobody had done it despite both LabVIEW and GDSII files being prevalent in physics...

ReadGDSIIFile (2).zip

Furthermore, the architecture would just change what parser is being used, as it takes a path as an input and sends back data. So we would just redirect it to the appropriate parser. And you can take a look at the dictionary for all of the different data types inside of the parser: (There are 44 apparently). image

ciozi137 commented 3 years ago

I see. So those 44 types would be like line, rectangle, polygon, etc. like I said I think we can get input from @aditi-nethwewala and @Nina-YDY about the most important objects to support. We don't need to support everything, especially not to begin with.

Monkeymerlot commented 3 years ago

Kind of? GDSII was created in 1978 so it isn't like a normal format that we would create today. It has some things like TAPENUM and TAPECODE that we obviously don't use today, but it's also mixed in with XY and PATHTYPE that we would use, as well as things like AREF and SREF which denote structure of the data. Looking at the spec again I don't think that it would be difficult to implement everything, but the difficulty would be in converting them into usable data, and discarding/working around things like AREF and SREF appropriately.

Here is the spec

If I could see a file that someone is actually using on the EBL it would probably help. If you want I can take a deeper look into this and open up another issue where we could focus on this in greater detail. It would also help if you could elaborate on what we want to do with the GDSII files. Are they to be used with AFMs to compare the effectiveness vs. EBL, do we just want a viewer for EBL, do we want to use the Litho program for EBL, or are we moving to GDSII from SVG.

Another thing to consider is if we are moving from SVG, what other files types could we use. OASIS looks interesting but upon a glance looks really difficult to implement because the byte size of the numbers changes.

ciozi137 commented 3 years ago

Posting some internet searches

ciozi137 commented 3 years ago

GDSII is de facto standard for EBL and also for photolithography masks. They are made using KLayout (previously we used LayoutEditor but we've moved on finally...). I'm thinking it makes most sense to use KLayout for AFM patterns as well.

Monkeymerlot commented 3 years ago

I actually have used Klayout before in Frolov's class so I knew what they are used for in the cleanroom etc. but wasn't sure what we were using them for in the lab. That makes sense, and I think that it is definitely worth it to write a parser for GDSII files so we could use either SVG or GDSII.

I also just took a look in KLayout and the only thing that would really be different in Klayout is that I cant seem to find a way to save images or image paths to the GDSII file. Could just be how I am saving them though.

I'll take a more detailed look at the feasibility of writing a GDSII parser tomorrow.

aditi-nethwewala commented 3 years ago

Here is the link to one of my gdsii files. I have mainly been using polygons: which can be of random shapes (circle, rectangles, mix of all...) and lines

ciozi137 commented 3 years ago

Aditi_GDSII.zip

Monkeymerlot commented 3 years ago

There are a lot of different layers in this file. Is there an organization to these layers ie what do we want to use for writing? image

Monkeymerlot commented 3 years ago

Re: Glue Code: I need to add logic to convert horizontal and vertical Lineto commands to normal Lineto commands. The coordinates are fine, but the command type will cause problems.

Fix: image

Monkeymerlot commented 3 years ago

Next step: Path, Rectangle, Funnel Settings located in an SMO that acts like a database. We can find any shared resource generically without having to be an SMO: image

This would help get rid of the global variables and monster clusters for the settings.

Monkeymerlot commented 3 years ago

Here is the flow I was thinking: image

Monkeymerlot commented 3 years ago

@ciozi137 I have a rough working SMO for implementation of this but for paths only atm. I am thinking about opening another branch and seeing how well this works with paths...

Class Hierarchy (Only the "Object" Class + Decendents and the singular SMO) image

On the Refresh paths (where you send it a number of parsed paths to create new objects for) we will use the previous settings, and if there are more new paths than the previous batch, then we will use the default settings (not yet implemented) for the paths that go over the amount of the old ones: image

Edit: This will also fix the (rarely, but sometimes encountered) bug when someone has more than 100 funnels, paths, or rectangles. Here is a highlight execution for this and you can see that they just use an initialized array of size 100 or 101. If you have more than that the program freaks out. image

Monkeymerlot commented 3 years ago

New dialogue using events:

Note that this is a mockup (not even proof of concept) Setting Dialogue Box image Update information in database image User selected a different path image

Previous dialogue using globals:

Setting Dialogue Box/Update Information image User selects a different path image

ciozi137 commented 3 years ago

@Monkeymerlot is "Path Settings" a class wire?

Monkeymerlot commented 3 years ago

@ciozi137 Yes. It inherits from Object Settings: https://github.com/levylabpitt/AFM-Lithography/issues/16#issuecomment-774370749

Edit: So is NewCurrentPath, which is the same class as Path Settings

ciozi137 commented 3 years ago

ok I see.

Monkeymerlot commented 3 years ago

Inside of Schedule Selector: image Inside the Formula Edit Dialogue: image

I have a class wire there for object settings, but I actually ended up not using it because I wanted this to be general between voltage and speed settings. I'm going to keep it there just in case we need it in the future...

Monkeymerlot commented 3 years ago

For the SMO based parser interface glue, my last correction was not actually sufficient. The command was corrected but there was still a matter of the actual points: image

Monkeymerlot commented 3 years ago

New UIs: image image image

Monkeymerlot commented 3 years ago

@ciozi137 I want to implement the parser in the main project. I should make a new branch, get it working, and then merge into develop correct?

ciozi137 commented 3 years ago

Yes that sounds good

Monkeymerlot commented 3 years ago

@ciozi137 Ok that was actually a pretty painful experience (there was a case structure that was case sensitive match and it was causing a bug wayyy down the road), but I think that it is good now. Could you take a look sometime tomorrow (Thursday)? Basically just run it and make sure that it is working correctly on your machine?

ciozi137 commented 3 years ago

@Monkeymerlot I didn't look too closely at this yet. I was able to open it and don't see any broken run arrows.