Qr cnc svg

[spatcho]

What is this PR for?

The existing functionality to export a qr code using the "cnc/file" printer driver was broken. It expected a QR code structure of 0, 1 and \n that wasn't used anymore. This PR adapt file cnc.py to the new optimized QR code array structure. It mainly consist of comparing to bits instead of chars, detecting new rows from the size of the qr instead of catching a '\n'. This was inspired from what was done in thermal.py code.

On that same cnc.py file there was a commented section how to export a qr code using a "cnc/grbl" printer driver. This would communicate directly to a grbl cnc via serial communication. That code was uncommented, tested against an Openbuilds 1515 grbl cnc using the BlackBox x4 controller and adapted to communicate successfully with the cnc. The file krux_settings.py was also modified to include the "cnc/grbl" driver option in the printers menu.

A possibility to export an encrypted QR code to svg format and not only to bmp or bpm was added. With svg you can easily include the QR code for 3d printing or cnc machining. The file qr_view.py add the necessary code to handle the QR code array and export as a svg file, navigating rows and cols like it's done for the cnc export, plus the menu entry so that the option appear when you want to export the encrypted qr. File sd_card.py add the svg image extension support.

Changes made to:

• [X] Code
• [ ] Tests
• [ ] Docs
• [X] CHANGELOG

What is the purpose of this pull request?

• [X] Bug fix
• [X] New feature
• [ ] Docs update
• [ ] Other

[qlrd]

@spatcho, reviewed a little your changes and there're some broken tests:

FAILED tests/printers/test_cnc.py::test_print_qr_code_with_row_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_with_spiral_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_inverted - TypeError: unsupported operand type(s) for &: 'str' and 'int'

I would be nice if you can be more specific in the PR about the changes.

[spatcho]

@spatcho, reviewed a little your changes and there're some broken tests:

FAILED tests/printers/test_cnc.py::test_print_qr_code_with_row_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_with_spiral_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_inverted - TypeError: unsupported operand type(s) for &: 'str' and 'int'

I would be nice if you can be more specific in the PR about the changes.

How much more specific ?

[tadeubas]

Hey @spatcho you are aiming MASTER / MAIN branch, it is wrong, we first aim for DEVELOP then later in time we merge DEVELOP into MASTER / MAIN creating a new version, this is why you didn't see other changes on CHANGELOG file for example.

Please just edit you PR and change the branch like this one in the img below:

[qlrd]

@spatcho, reviewed a little your changes and there're some broken tests:

FAILED tests/printers/test_cnc.py::test_print_qr_code_with_row_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_with_spiral_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_inverted - TypeError: unsupported operand type(s) for &: 'str' and 'int'

I would be nice if you can be more specific in the PR about the changes.

How much more specific ?

I advocate for verbosity. No need to explain like im 5 years old, but i'm trying to understand the firmware too (i came from installer and studying firmware to help with tests).

So i think if (new potentially) developers could know your rationale would be a great plus.

[spatcho]

@spatcho, reviewed a little your changes and there're some broken tests:

FAILED tests/printers/test_cnc.py::test_print_qr_code_with_row_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_with_spiral_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_inverted - TypeError: unsupported operand type(s) for &: 'str' and 'int'

I would be nice if you can be more specific in the PR about the changes.

How much more specific ?

I advocate for verbosity. No need to explain like im 5 years old, but i'm trying to understand the firmware too (i came from installer and studying firmware to help with tests).

So i think if (new potentially) developers could know your rationale would be a great plus.

Please have a look now.

[spatcho]

Hey @spatcho you are aiming MASTER / MAIN branch, it is wrong, we first aim for DEVELOP then later in time we merge DEVELOP into MASTER / MAIN creating a new version, this is why you didn't see other changes on CHANGELOG file for example.

Please just edit you PR and change the branch like this one in the img below:

It's done, you confirm ?

[qlrd]

@spatcho, reviewed a little your changes and there're some broken tests:

FAILED tests/printers/test_cnc.py::test_print_qr_code_with_row_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_with_spiral_cutmethod - TypeError: unsupported operand type(s) for &: 'str' and 'int'
FAILED tests/printers/test_cnc.py::test_print_qr_code_inverted - TypeError: unsupported operand type(s) for &: 'str' and 'int'

I would be nice if you can be more specific in the PR about the changes.

How much more specific ?

I advocate for verbosity. No need to explain like im 5 years old, but i'm trying to understand the firmware too (i came from installer and studying firmware to help with tests). So i think if (new potentially) developers could know your rationale would be a great plus.

Please have a look now.

Amazing, now i understood what you want/ and it's great!

[odudex]

Time to refine the PR run poetry run poe black to format your code changes. run poetry run poe lint to run pylint to check some code compliance parameters, then you have to address the issues pointed by it. run poetry run poe test to run our automated tests. Each broken test must be evaluated, sometimes the code is flawed, other times the test itself must be adapted. Finally, there will be a test coverage test, where we ensure at least 90% of new code is covered by tests.

[qlrd]

Time to refine the PR run poetry run poe black to format your code changes. run poetry run poe lint to run pylint to check some code compliance parameters, then you have to address the issues pointed by it. run poetry run poe test to run our automated tests. Each broken test must be evaluated, sometimes the code is flawed, other times the test itself must be adapted. Finally, there will be a test coverage test, where we ensure at least 90% of new code is covered by tests.

@odudex, I managed to re-adapt the tests at certain point, to accept bits instead string, but the results made by src/krux/cnc.py::GCodeGenerator::print_qr_code differs from the expected result in tests/printers/files/*.nc. But at same time @spatcho made some changes (i think i used them, but not sure).

So i dont know if the expected result is wrong and need a change.

Made a PR in his repo to you, it's a lot of expected results in some .nc but none of them matched.

[spatcho]

New commit. I guess I did all you asked, up to https://github.com/selfcustody/krux/pull/557#issuecomment-2787204736 .

[odudex]

Ok, now that I merged other pull requests, this branch has some conflicts to be solved. The PR also have some changes in unrelated files. Since this PR is also for learning (it's @spatcho 's first PR, and we're also learning how to manage multiple development fronts), I would suggest to restart the PR from scratch, or better, create 2 PRs, one at a time. The code, of course, should be reused, @spatcho can make a backup elsewhere, then do the following. Fist, let's do a simpler PR, I believe it would be the .svg export Check out to develop branch, run git fetch origin, git checkout develop, git submodule update --recursive and ensure your git tree is clean. Create a new branch, like svg_export, and

• Insert the code required to make svg export to work.
• Edit changelog and add respective information. (We can help you stage and commit only changes in files that are related to the PR)
• Run poetry run poe black, poetry run poe lint, poetry run poe test. We'll help with any issue that you might have.
• Only after everything is green, we do a final review and merge the "SVG Export" PR.

After this we do the same with a "CNC Fix and Update"

What do you think?

[qlrd]

Agreed, there's a lot of ways to manipulate the git history so he do not lost his work.

I think with some git add --patch, rebase, fix and drops is possible to separate this in two PRs without type any code

[tadeubas]

Ok, now that I merged other pull requests, this branch has some conflicts to be solved. The PR also have some changes in unrelated files. Since this PR is also for learning (it's @spatcho 's first PR, and we're also learning how to manage multiple development fronts), I would suggest to restart the PR from scratch, or better, create 2 PRs, one at a time. The code, of course, should be reused, @spatcho can make a backup elsewhere, then do the following. Fist, let's do a simpler PR, I believe it would be the .svg export Check out to develop branch, run git fetch origin, git checkout develop, git submodule update --recursive and ensure your git tree is clean. Create a new branch, like svg_export, and

• Insert the code required to make svg export to work.
• Edit changelog and add respective information. (We can help you stage and commit only changes in files that are related to the PR)
• Run poetry run poe black, poetry run poe lint, poetry run poe test. We'll help with any issue that you might have.
• Only after everything is green, we do a final review and merge the "SVG Export" PR.

After this we do the same with a "CNC Fix and Update"

What do you think?

I like to use the poetry run poe pre-commit as it executes the black, lint, check for new translations and tests all in one simple command.

IMO it is simpler to just do the merge, he already removed the unnecessary changes to unrelated things. If @spatcho wants I can resolve the conflicts and create a PR to his branch in its fork and when accepted this PR here will be automatically updated.

[spatcho]

Ok, now that I merged other pull requests, this branch has some conflicts to be solved. The PR also have some changes in unrelated files. Since this PR is also for learning (it's @spatcho 's first PR, and we're also learning how to manage multiple development fronts), I would suggest to restart the PR from scratch, or better, create 2 PRs, one at a time. The code, of course, should be reused, @spatcho can make a backup elsewhere, then do the following. Fist, let's do a simpler PR, I believe it would be the .svg export Check out to develop branch, run git fetch origin, git checkout develop, git submodule update --recursive and ensure your git tree is clean. Create a new branch, like svg_export, and

• Insert the code required to make svg export to work.
• Edit changelog and add respective information. (We can help you stage and commit only changes in files that are related to the PR)
• Run poetry run poe black, poetry run poe lint, poetry run poe test. We'll help with any issue that you might have.
• Only after everything is green, we do a final review and merge the "SVG Export" PR.

After this we do the same with a "CNC Fix and Update" What do you think?

I like to use the poetry run poe pre-commit as it executes the black, lint, check for new translations and tests all in one simple command.

IMO it is simpler to just do the merge, he already removed the unnecessary changes to unrelated things. If @spatcho wants I can resolve the conflicts and create a PR to his branch in its fork and when accepted this PR here will be automatically updated.

@tadeubas if you could do the resolve as proposed I would much appreciate it

[odudex]

That works too! I still see changes in .gitignore and PR templates that needs to be undone.

[spatcho]

That works too! I still see changes in .gitignore and PR templates that needs to be undone.

About .gitignore : I replied to qlrd comment here https://github.com/selfcustody/krux/pull/557#discussion_r2033883678 , is it about something else ?

About PR templates : what is it ?

[qlrd]

That works too! I still see changes in .gitignore and PR templates that needs to be undone.

About .gitignore : I replied to qlrd comment here #557 (comment) , is it about something else ?

About PR templates : what is it ?

PR/issue templates are files that are used when someone opens a PR/issues.

You remember when you created this PR and the related issue and had some pre-filled text? it's that.

[spatcho]

I pushed a reverted .gitignore. I will try to keep mine unpushed. I want to close this PR.

[spatcho]

That works too! I still see changes in .gitignore and PR templates that needs to be undone.

About .gitignore : I replied to qlrd comment here #557 (comment) , is it about something else ? About PR templates : what is it ?

PR/issue templates are files that are used when someone opens a PR/issues.

You remember when you created this PR and the related issue and had some pre-filled text? it's that.

And what shall I undone ?

You mean that ?

[qlrd]

That works too! I still see changes in .gitignore and PR templates that needs to be undone.

I believe it's worth to a rebase too, see that are two unrelated commits from @tadeubas and one from @odudex. You have telegram or discord (with @tadeubas too)? maybe we can do togheter there, so we don't fill up too many comments here .

[qlrd]

That works too! I still see changes in .gitignore and PR templates that needs to be undone.

About .gitignore : I replied to qlrd comment here #557 (comment) , is it about something else ? About PR templates : what is it ?

PR/issue templates are files that are used when someone opens a PR/issues. You remember when you created this PR and the related issue and had some pre-filled text? it's that.

And what shall I undone ?

You mean that ?

A rebase resolve that, if you wanna, find-me on Krux group and lets talk

[spatcho]

I can send you or you can extract patch files from my work on include it in the project.

[odudex]

In my opinion, two new, smaller PRs from scratch, without overwhelming git maneuvers, and where @spatcho understand every step, is safer and more didactic. The same applies to poetry run poe pre-commit, at least for a few times, it would be good for a new contributor to run black, lint, and tests separately, understand what's happening, learn about the importance, and get familiarized with each tool. I don't know if @spatcho just wants to get this feature working and move to other things, but I hope this is just the first of many contributions. Krux is a great platform to implement innovative ideas, learning more about the tools Krux has, in the long term, will make him accomplish his goals even faster.

[spatcho]

I just wanted to contribute my changes as a thank you for a great project. I will probably have more minor changes later and maybe a gcode generation specific for laser when I have the equipment. I appreciate to be known as a contributor but mainly I wish that my changes are integrated in the project. This start to be heavy on me, I could agree to submit new PRs, but I have the impression that it could easily start the loop talk again.

[odudex]

Loop talks and focus deviations are exactly what I would like to avoid. For that, I think the best strategy is to keep it simple, break things that are starting to fold on itself in small peaces, finish, and move them behind. Not introducing any new topic, except minimum requirements. The goal is simple, to have the code fully compliant. Let me know if you need any help.

[tadeubas]

Hi guys, I believe the changes in .github/PULL_REQUEST_TEMPLATE.md are just because it started in main and is now in develop, so we changed that template and forgot to merge main into develop, but Git is smart and detects and merges the changes later, and if there is a conflict we can always merge manually too.

We have had a lot of PRs changing .gitignore, it is not a file that needs to be optimized, it is not integrated into the firmware, it is just a file to help developers. I was just wondering what the additions were and if they were necessary, of course we should add *.code-workspace (and any other IDE specific files) and */.DS_Store (for Mac users).

@spatcho has already said he does not use Git on the command line, Git is not easy and can be a hassle and a burden. He just wants to close this PR (meaning he wants this merged). It will go to develop, so I guess we can iterate on the code later, since we are more familiar with the code being fully compliant and it is clear to me that these requests are taking the joy out of coding for the new contributor.

I will merge it with rebase (with the help of @qlrd of course, because I am not used to it). Afterwards, I promise to add something basic to the documentation mentioning this feature. After a long time living in the firmware, this CNC feature has been really tested and improved. Thanks @spatcho for your work and testing! I hope you continue contributing to Krux in the near future! I will create the PR to your fork and when you accept this PR here will be automatically updated.

[odudex]

so we changed that template and forgot to merge main into develop

Good catch. Merged main into develop

[qlrd]

@spatcho, you work is awesome. I think you will be a great addition!

I know git could be hard at first, and was for everyone on the team. I hope you can understand what we want to achieve is simplicity and readability on reviews.

Hi guys, I believe the changes in .github/PULL_REQUEST_TEMPLATE.md are just because it started in main and is now in develop, so we changed that template and forgot to merge main into develop, but Git is smart and detects and merges the changes later, and if there is a conflict we can always merge manually too.

We have had a lot of PRs changing .gitignore, it is not a file that needs to be optimized, it is not integrated into the firmware, it is just a file to help developers. I was just wondering what the additions were and if they were necessary, of course we should add *.code-workspace (and any other IDE specific files) and */.DS_Store (for Mac users).

@spatcho has already said he does not use Git on the command line, Git is not easy and can be a hassle and a burden. He just wants to close this PR (meaning he wants this merged). It will go to develop, so I guess we can iterate on the code later, since we are more familiar with the code being fully compliant and it is clear to me that these requests are taking the joy out of coding for the new contributor.

I will merge it with rebase (with the help of @qlrd of course, because I am not used to it). Afterwards, I promise to add something basic to the documentation mentioning this feature. After a long time living in the firmware, this CNC feature has been really tested and improved. Thanks @spatcho for your work and testing! I hope you continue contributing to Krux in the near future! I will create the PR to your fork and when you accept this PR here will be automatically updated.

Yeah, this is what happened. Rebase is a bad idea althought (i tried here and had 98 conflicts).

I think a better approach is branch a qr_cnc and a qr_svg from develop by reseting it to current state of develop, start to cherry-pick each one of the commits, resolve conflicts one by one (3 commits, i will need the help of @tadeubas to select which changes are the correct) and add --patch/ stash to separate the PRs.

So we will not lost @spatcho awesome work neither his authoring.

[qlrd]

@spatcho, i think i managed to separate the CNC code. Could you review to see if i didnt forget anything?

https://github.com/selfcustody/krux/compare/develop...qlrd:krux:qr_cnc?expand=1

[qlrd]

@spatcho, can you review and test https://github.com/selfcustody/krux/pull/562 ?