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

Replace switch-case statement with config file for better management #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

coded-with-claws
Copy link

Hi,
Could you please test the new code on a real MisTer because I don't own the RFID hardware needed. I just tested the code on a Linux machine to verify it tries to run the good rom.
Also, we could then envision an evolution of ElRojo's fork so that he writes into the config file :)

@coded-with-claws
Copy link
Author

This new version of the script works well on my MiSTer when I manually run rfid_process.sh so it should work well with the real RFID reader :)

@javiwwweb
Copy link
Owner

javiwwweb commented Aug 19, 2022

Nevermind my last comment (went ahead and deleted it) The scripts works great, but it only works if its started manually. There's an issue with forwarding the card info the script generates to the "screen" session. If you connect to the screen session it starts working again,

@coded-with-claws
Copy link
Author

coded-with-claws commented Aug 20, 2022 via email

@coded-with-claws
Copy link
Author

coded-with-claws commented Aug 21, 2022

Hi,
Thanks for your update!
I'll try to figure it out about this weird behavior with the serial_listen / screen.
In the meantime, I commited little changes :

  • absolute path for config file (following your feedback, and it seems logical because of the organization of "serial_listen => bash => executes what arduino says", as arduino doesn't run from directory /media/fat/Scripts/
  • don't run any game if RFID is not found (I had a bug: if RFID not found, it ran the last game of config file). I think that all the executions of "rfid_process.sh noscan" from the arduino could have tried to run the last game again and again, because of that bug.

In case you wonder why I added an "unset" command: I have seen that arduino runs ". rfid_process.sh" instead of just "rfid_process.sh". This means that the bash variables of rfid_process.sh keeps getting sourced, aka they persist in the bash session of serial_listen.sh.
Could you please test this new version of rfid_process.sh in case it would make things better, or at least more understandable regarding the remaning problem with screen session ?

@javiwwweb
Copy link
Owner

javiwwweb commented Aug 21, 2022 via email

@ElRojo
Copy link

ElRojo commented Sep 27, 2022

Hey guys, I'm just getting caught up here! I like the config file idea. That should be something I could merge into my repo. Thanks to both of you. I'm loving that this is taking shape :)

@ElRojo
Copy link

ElRojo commented Sep 28, 2022

Hey @coded-with-claws, I loved your approach and got it to work on my fork. Before I merge this PR, though, would you like to make a PR on my repo with those changes so that you get proper credit? Or would you just like some credit in the ReadMe? I want to make sure you're properly thanked! :D

Here's the PR: ElRojo#18

@coded-with-claws
Copy link
Author

Hi @ElRojo, thanks for your interest for the config idea! I'd love that you could merge it into your repo!
It's ok for me that you just include the code and cite me into the Readme :)
Please note that I still couldn't test the config file on a real MiSTer, I got a friend who has prepared the RFID reader for me, it's being shipped, I'll be able to test it soon!
As the conf file in your PR is empty, maybe you could say in the readme that we must write per line the id then a tabulation then the command line (rom), and also maybe give an example ?
I hope it works great on yours!
Thanks to your PR, I'll directly upload your code on my MiSTer + RFID when I get it, thus I'll have the config file :)

@ElRojo
Copy link

ElRojo commented Sep 28, 2022

@coded-with-claws I'll get some more explicit citation added for you!

I can add some information on how to manually add games -- though this version doesn't require manually adding!

Lastly, I have indeed tested it and it's working great. Well done!!

@coded-with-claws
Copy link
Author

@ElRojo I'm so glad that you tested it and that it's working great!
I just read the details of your PR, everything seems ok as you automatically add the games into the config file (so the format will be ok), and you added me into the "thank you" section :)
Thanks for having cared about whether I wanted a PR and credit :) I'm happy of the credits in the readme :)
I can't wait to deploy those files on my mister when I get the RFID reader!

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