EmixamPP / linux-enable-ir-emitter

Provides support for infrared cameras that are not directly enabled out-of-the box.
MIT License
242 stars 21 forks source link

Auto config #8

Closed EmixamPP closed 3 years ago

EmixamPP commented 3 years ago

On the auto-config branch, you can see an auto directory, as well as inside a python script auto-enable-ir-emitter.py that will test each configuration of config.json. If the C script runs without error, the user should confirm that the infrared emitter is working. If yes, the python script proposes to create the systemd service.

I will test the program a few more days and if there are no problems I will merge it with the master Edit: the branches have been merged

@renyuneyun You proposed to make an AUR package, personally I'm on Manjaro but I don't know if I'm going to be there for long, so it might be better if you took care of it. I don't know if I should create a new repo? Or this one is enough and I put you as a collaborator? Or we use your repo, I don't mind.

Any other help or comment is of course welcome.

renyuneyun commented 3 years ago

That's good to know the auto-config thing! I'll give it a test and report back. There are some ambiguous details for the moment.

If you are fine with that, I can maintain the AUR package. If noticed or it has been flagged out-of-date, I'll try to keep it up to date with the latest releases.

renyuneyun commented 3 years ago

Err, just a second, I don't see that auto-config branch in the repo (actually I only see the master branch). Was it deleted or forgotten to be pushed? I see there is the auto directory in the master branch.

renyuneyun commented 3 years ago

I'm reporting back after testing it. First of all, I have no comment about the C code itself, because it works perfectly on my machine.

The "auto" scrip also works fine. But in my understanding, the "auto" script is on the opposite of what is expected in a package... I mean, it is meant to interactively perform everything for compiling and configuring, and even tries to modify /. This is useful for human-watched installation, but doesn't work well for automatic building and installation. It will also make it hard to uninstall this application.

I can imagine a quick "fix" resulting in a new procedure like this:

  1. Make the auto-config script try different settings, and print the expected one out.
  2. Have a configuration file containing the expected arguments that are printed out by the auto-config script.
  3. When executing the main program, read from the configuration file, and perform accordingly.
  4. Change the .service file to run the main program directly, rather than having the exact arguments in it.

In this way, the packaging process can be:

  1. Compile the main program
  2. Include the auto-config in the package
  3. Include the main program in the package
  4. Include the .service file in the package
  5. (Optionally) Include a dummy/default configuration file in the package
  6. (Optionally) When installing the package, print a message asking the user to run the auto-config first, and then add the information to the configuration file

Therefore, when the user installs the package, what he needs to do is:

  1. Execute the auto-config provided.
  2. Add the printed configuration to the configuration file.
  3. Execute the main program or enable/start the service.

In this way, only 1 file needs to the modified by the user (automatically or manually, which can be discussed), which is the configuration file. In my understanding, this aligns a lot better to packages in the Linux world.

How do you feel about that?

EmixamPP commented 3 years ago

"the auto script is on the opposite of what is expected in a package". I totally agree with you, but I had preferred this approach for my repo.

I had imagined that the package, would be in fact just a download of the auto folder and thus with the possibility of executing the python script in the terminal. Then the user uses it as if he had git clone from the repo. Basically, I had imagined a utility program that you run once and then uninstall.

But I can indeed modify what I did to take into account your remarks! I can make a branch only dedicated to packaging.
I'll do this quickly when I find some free time.

And the only thing that could change in this package is the number of shared configurations for users who have set up the script by hand.

Thank you for your help!

EmixamPP commented 3 years ago

I pushed on package branch in the package folder several things :

EmixamPP commented 3 years ago

The systemd service don't work. I don't understand why but I will try to fix it today. I have never done a template service before, the problem must be there.

edit : fix, I have edited my previous post

renyuneyun commented 3 years ago

I saw the comment last week but only got time to work on it recently. I created a PR (#12) which is based on the stuffs in the package branch.

I believe the package directory is solely for development and testing, and will be put to the upper directory later, right? If so, because you are already using branches, probably you want to directly put them in the upper directory. This makes it neater and easier to keep track of changes.

By the way, depending on your intention, you may consider YAML or TOML as well. YAML is generally a better choice than JSON if manual modification to the file is expected. TOML is more intuitive and also expressive. After all, JSON is designed for network communication, not for configuration nor human-oriented.

EmixamPP commented 3 years ago

Indeed, I had simply used the JSON, because at that time I was on another project which used a JSON serializer. I will change this. See the PR for the rest of the discussion

EmixamPP commented 3 years ago

Regarding the script (for automatically testing and running), it depends on you. After all, it's the utility, not the core program. Depending on your final modifications to that script, I may create another script with the packaging scripts (PKGBUILD) for the purpose of executing it.

For me it is finished, maybe I will add in the help section, the mention of the systemd service available. Edit: done

I'm not sure what do you mean by "compile the bash script". Do you mean something like "automatically generate the bash script (probably linux-enable-ir-emitter?) depending on the user's expectation through Makefile"?

Oh no, it's much simpler! In fact, the idea came to me when I saw that the bash script was placed in .../bin so for binaries. With the shc utility you can compile a bash script to become a binary: shc -f script.sh -o binary.

So, for me, everything looks good. Thanks again for what you do, it's awesome.

renyuneyun commented 3 years ago

Ah, good to know. I saw you made further updates to the master branch. Hope to see the two branches merged soon :) I published the package to AUR (from source). Please let me know when the two branches are "unified" (or some other way), and I can update the package accordingly.

For compiling scripts, I actually never knew the shc utility before, though can imagine something like that. It looks like it's for obfuscation or special needs (e.g. protecting against modification, according to wikipedia). I would consider it's unnecessary, unless there is a significant performance difference (I believe won't). Unless for protection in the first place (e.g. not want to open source), people should be encouraged to use permissions to "protect" script (/file) content.

EmixamPP commented 3 years ago

(Yes compiling the bash script is totally unnecessary in our case. It was just to keep the /bin convention.)

OK so :

I know you wanted all the files to be in the root, but it seems messy to put it all at the root of the master. And I'm afraid that the people who come on my repo might be a bit lost.

I hope what I have done is not a problem?

renyuneyun commented 3 years ago

Sure, having them in sub-directories are totally fine. I'll update the AUR package for this when I have time. (As always, if you [or anyone] are interested, I can hand over the package to you.) I totally agree that having them in the root directory can lead to a mess. Splitting them in different subdirectories makes sense.

Just one more question: do you plan to have two versions (manual and auto) in the long run, or do you plan to merge them at some point?

EmixamPP commented 3 years ago

I'm fine with you keeping the package. Conversely, if you prefer to give it to me, there is no problem!

In the future I plan to develop an even more advanced configuration tool (which will only require a few manipulations and will be able to configure any camera). But I still need to gather my ideas and information so that I don't overkill something. I'm not sure if I'm going to be able to do this, but I'll have to do it myself, and I'm not sure if I'm going to be able to do this. And it will probably be a complete program. Maybe a graphic sotware, so that it is extremely easy to use. (And maybe more in the spirits of "Windows" than "Linux".)

So, to answer your question, I think that in the future the package could change. I tend to go for a "utility", rather than a script that runs silently in the background.

However, I think I'll keep every possibility in the future. Since the manual configuration requires no maintenance for me. And the current automatic one, I just have to add lines in a YAML file which is not very long. This way everyone can continue to find what they are looking for.

EmixamPP commented 3 years ago

At the end of the day, I'm going to stay with what we've been doing. The only difference is that I'm going to do everything in python instead of bash.