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

Added configure.sh and accompanying doc #82

Closed
wants to merge 25 commits into from

Conversation

CodeCanna
Copy link
Contributor

Ready for review and testing.

@atar-axis
Copy link
Owner

atar-axis commented Mar 13, 2019

will give it a try tonight, thank you very much so far :)
btw, codacy complains about an unused ARGS variable, it it a bug or is it codacy which is wrong?

@atar-axis
Copy link
Owner

atar-axis commented Mar 13, 2019

The script is not able to detect my (installed, but not active) xpadneo instance:

flood@flood-r3:[xpadneo]$ sudo ./configure.sh 
[sudo] Passwort für flood: 
Installaion not detected, did you run install.sh?

I guess this is because I had an older version installed - therefore you should also check for older versions and complain if the version does not fit the configure script :)

Btw, there is a typo: "Installaion"

Another thing is, that we should maybe additionaly permanently change the options using modprobe, as described in the README - this does also work if the module is not loaded :)

@CodeCanna
Copy link
Contributor Author

Should the script run if a different version is detected?

@atar-axis
Copy link
Owner

No, I think it would be enough to report an error - otherwise we would have to check if a given parameter is available in the installed version. That's too much I think :)

@CodeCanna
Copy link
Contributor Author

CodeCanna commented Mar 13, 2019

Okay, so now the script checks if the version is the same as the number in the VERSION file in the xpadneo directory, and throws a warning before exiting.

@atar-axis
Copy link
Owner

Perfect, the only thing missing now is the permanent change in /etc/modprobe.d/xpadneo.conf 😊

@CodeCanna
Copy link
Contributor Author

So just to be sure, in order to make changes permanent you put options hid_xpadneo debug_level=3 in xpadneo.conf. If I want to do disable_ff I would put options hid_xpadneo disable_ff=true and so on, is it all the same for each option?

@atar-axis
Copy link
Owner

@atar-axis
Copy link
Owner

atar-axis commented Mar 14, 2019

To take it short: yes! The only thing you need to take care about is to not overwrite existing options - you may need to parse an existing xpadneo.conf therefore in advance to preserve settings which are already there.

@CodeCanna
Copy link
Contributor Author

CodeCanna commented Mar 14, 2019

I'm a little confused about parsing the conf file. I was thinking of using echo to put the appropriate line into the conf like:

echo "options hid_xpadneo debug_level=2" >> /etc/modprobe.d/99-xpadneo-bluetooth.conf

But I think that would cause duplicate lines if that option was changed more than once. Is that why you use tee in the example?

will give it a try tonight, thank you very much so far :)
btw, codacy complains about an unused ARGS variable, it it a bug or is it codacy which is wrong?

Also sorry, I didn't see the rest of this post, I didn't see where you asked about the variable. It was the $ARGS variable which didn't end up being used.
And thank you for letting me work on this, I this is the first thing I have found that I can put my coding to actual work, It means a LOT to me I have been stagnant and not knowing what to code; and have been trying to get into open source contributions for while.

@CodeCanna
Copy link
Contributor Author

Ah looks like I can use sed for this. A new command for me.

@CodeCanna
Copy link
Contributor Author

I need an opinion, should I add all of the config lines in /etc/modprobe.d/xpadneo.conf when the script runs. Because the only line that is in there by default is options bluetooth disable_ertm=y, and sed won't edit the line if it doens't exist. So I have come up with either echoing all the config lines into the file when the script is ran, or using grep to check if the line exists or not, and if not to create it so sed can edit it.

@atar-axis
Copy link
Owner

atar-axis commented Mar 15, 2019

It means a LOT to me

I can remember that time :)
I am happy that it is fun to you too

Ah looks like I can use sed for this

Right! sed is the way to go I think, or perl - which regex syntax is a bit easier. Parsing the config file is maybe the Most difficult part of this Script

I need an opinion

options bluetooth ... has nothing to do with the xpadneo configs, it's just in the same file.
I think thats clear, right?

So I have come up with either echoing all the config lines into the file when the script is ran, or using grep to check if the line exists or not, and if not to create it so sed can edit it.

The latter option sounds better to me - in the end it is up to you, as long as the solution is robust and clean ;)

@CodeCanna
Copy link
Contributor Author

CodeCanna commented Mar 15, 2019

options bluetooth ... has nothing to do with the xpadneo configs, it's just in the same file.
I think thats clear, right?

No the file has that line in it by default. When I opened the config file that line was already in there. Is it supposed to be empty?

EDIT: Also I agree that the best way is to use grep, but I have to put that in every option. I was thinking of making a function that does that and call that function, but I don't seem to understand how to pass parameters in functions in bash.

Do you know how I could put this:

if grep -q "trigger_rumble_damping" "$CONF_PATH"
    then
      # If line exists modify it.
      echo $NAME:"Value written to $CONF_PATH"
      sed -i 's/options hid_xpadneo trigger_rumble_damping=.*/options hid_xpadneo trigger_rumble_damping='$val'/' $CONF_PATH
    else
      # If line does not exist, create it.
      echo $NAME:"Value written to $CONF_PATH"
      echo "options hid_xpadneo trigger_rumble_damping=$val" >> $CONF_PATH
    fi

in a function so I don't have to type this out for each option. I know how to basically make a function in bash but I don't know how to make this function work with all the different options because I can't pass parameters like in a normal programming language.

EDIT: Something like this seems to work, for figuring out what argument was passed to the script:

function change_line() {
	case $ARG in
		"Noodle" | "noodle")
			echo "This function will work on Noodle";;

		"Crayon" | "crayon")

			echo "This function will work on Crayon";;

		"Apples" | "apples")

			echo "This function will work on Apples";;

		"Spaghetti" | "spaghetti")

			echo "This function will work on Spaghetti";;

		"Cereal" | "cereal")

			echo "This function will work on Cereal";;

			*)

			echo "Arg not recognized"
	esac
}

But I need to see if I can use grep to recognize what line to edit based on the argument passed, which is where my dilemma is.

@atar-axis
Copy link
Owner

Everything is alright, this Line is in there to disable the faulty ERTM in Linux which prevents a sucessful connection with the gamepad 😊

@CodeCanna
Copy link
Contributor Author

I might have found a better aproach to using bash to parse the config file. I have seen that Python has a good library for this. What do you think?

@atar-axis
Copy link
Owner

atar-axis commented Mar 16, 2019

I don't like the idea of adding another dependency for this little script - not everybody has Python installed. If you don't like Shell scripts, then I would suggest perl, which is installed in most (all?) Distributions. 😊

@CodeCanna
Copy link
Contributor Author

Hmm never thought about perl. Is it a big language in linux? I'll look into it. If that doesn't work and time isn't an issue I will figure something out. If time is an issue I have a method that will work, it will just be redundant. I'm just really trying to use a function instead of writing the same code for each option to parse the config file.

@atar-axis
Copy link
Owner

Time is not a problem - don't worry, that's the good thing in open source projects like this 😊

@CodeCanna
Copy link
Contributor Author

Awesome, I felt like I was taking too long. I'm glad, now I can take time to really learn some of these concepts. 😌

@atar-axis
Copy link
Owner

I opened this issue two month ago, take your time ;)

@CodeCanna
Copy link
Contributor Author

Alright!! Give that a try, I rewrote the entire thing. I discovered this awesome handy tool that everyone else probably knows about already called getopt. Made it soooo much easier and cleaner. And I stuck everything into functions this time. I had a good time writing this one! 😃

@atar-axis
Copy link
Owner

Yeah, just give the suggestion a try - it's not super important to do it this way but it looks a bit clearer.
I will give it a try a bit later

@CodeCanna
Copy link
Contributor Author

CodeCanna commented Apr 3, 2019

No really a reason, that's just the way I grabbed the version number from the file name. I will use the one in update.sh. It doesn't seem to be working for me though.

DETECTED_VERSION=$(dkms status 2>/dev/null | grep '^hid-xpadneo' 2>/dev/null | sed -E 's/^hid-xpadneo, ([0-9]+.[0-9]+.[0-9]+).*installed/\1/')

when i echo $DETECTED_VERSION, its empty. I'll fiddle with it, and see what happens :)

EDIT: I echoed the $INSTALLED in update.sh and it's also blank, I think I'm missing something. No rush though get back to me when you can :)

@atar-axis
Copy link
Owner

that's not so important, keep your way - we can do that later ;)

@atar-axis
Copy link
Owner

Hey hey, I will give it a try today :) Thank you for your work!

@atar-axis
Copy link
Owner

atar-axis commented Apr 7, 2019

I like it! It works like a charm so far :)
Haven't had a look into the code but I will do that later.
Thank you for your contribution so far!

One thing I would like to mention:
combined_z_axis=0 is the default value which you are echoing, but combined_z_axis=y/n is the one which you are replacing. both are fine, but just keep it consistent please :)

Are you going to fix the Shellcheck issues? Tell me if there is a reason to prefer the existing way over the one mentioned by codacy/code climate

Ah and another thing: If the user does run the script without any option then - at least in my eyes - the help message should be shown too, right? So ./configure.sh -h should behave the same as ./configure.sh

@atar-axis atar-axis self-requested a review April 7, 2019 08:27
@CodeCanna
Copy link
Contributor Author

I like it! It works like a charm so far :)

That is so great to hear!!

Thank you for your contribution so far!

It was my pleasure and my joy, I have learned SO much in the past month or so working on this. It's been great!

Are you going to fix the Shellcheck issues? Tell me if there is a reason to prefer the existing way over the one mentioned by codacy/code climate

There is no preference, that's just the only way I know to do it, but I would love to learn a better way. I tried copying this line from update.sh:

VERSIONS=($(dkms status 2>/dev/null | grep '^hid-xpadneo,' 2>/dev/null | sed -E 's/^hid-xpadneo, ([0-9]+.[0-9]+.[0-9]+).*/\1/'))

however I could not get it to work right. When I echo $VERSIONS its empty, I am probably not doing it right. I'll mess around with it again and show you what I am trying.

combined_z_axis=0 is the default value which you are echoing, but combined_z_axis=y/n is the one which you are replacing. both are fine, but just keep it consistent please :)

I can fix that no problem.

Ah and another thing: If the user does run the script without any option then - at least in my eyes - the help message should be shown too, right? So ./configure.sh -h should behave the same as ./configure.sh

This is a great idea, I will add this as well. I never thought of it but it is true that most commands show the help menu when no arguments are called.

@CodeCanna
Copy link
Contributor Author

So I took that line from the update.sh and put it in my script and echoed it and it is empty.

@atar-axis
Copy link
Owner

What's the Output of dkms status in your system

@CodeCanna
Copy link
Contributor Author

I'm out and about, I'll message you when I get home.

@CodeCanna
Copy link
Contributor Author

Output of dkms status:

Error! Could not locate dkms.conf file.
File: /var/lib/dkms/hid-xpadneo/0.5.4/source/dkms.conf does not exist.

Looks like I'm missing something. I'll see what I can find as far as a fix.

@CodeCanna
Copy link
Contributor Author

A few possible fixes are this and this. Unfortunately I don't know much about dkms yet, but I'll read through these to see if I can find anything.

configure.sh Outdated
NAME="$0"
OPTS=$(getopt -n "$NAME" -o hz:d:f:v:r: -l help,version,combined-z-axis:,debug-level:,disable-ff:,fake-dev-version:,trigger-rumble-damping: -- "$@") # Use getopt NOT getopts to parse arguments.

echo "$VERSIONS"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding an array without an index only gives the first element.

@atar-axis
Copy link
Owner

  1. Run the Uninstall Script
  2. Remove /var/lib/dkms/xpadneo...
  3. Run dkms Status again

@CodeCanna
Copy link
Contributor Author

Ok I ran the uninstall script and it said it found 0 instances of xpadneo.
I removed /var/lib/dkms/hid-xpadneo/
I ran dkms status and now it outputs nothing.

@atar-axis
Copy link
Owner

Perfect, run the Install Script now - it should Work then

@CodeCanna
Copy link
Contributor Author

Nice! It works now. I will replace that line and push that.

configure.sh Outdated
NAME="$0"
OPTS=$(getopt -n "$NAME" -o hz:d:f:v:r: -l help,version,combined-z-axis:,debug-level:,disable-ff:,fake-dev-version:,trigger-rumble-damping: -- "$@") # Use getopt NOT getopts to parse arguments.

echo "$VERSIONS"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible misspelling: VERSIONS may not be assigned, but VERSION is.


# Check if version is out of date.
function check_version {
if [[ "$VERSION" != "$DETECTED_VERSION" ]];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding an array without an index only gives the first element.

then
echo "$NAME:Your version of xpadneo seems to be out of date."
echo "$NAME:Please run ./update.sh from the git directory to update to the latest version."
echo "$DETECTED_VERSION"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding an array without an index only gives the first element.


## Version ##
function display_version {
echo "Xpadneo Version: $DETECTED_VERSION"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding an array without an index only gives the first element.

@codeclimate
Copy link

codeclimate bot commented Apr 7, 2019

Code Climate has analyzed commit b0d0c1a and detected 0 issues on this pull request.

View more on Code Climate.

@atar-axis
Copy link
Owner

Perfect :) I will merge your work into master tomorrow/later - I think it is ready now :) See ya later!

@CodeCanna
Copy link
Contributor Author

Sounds great to me, thanks for you patience and letting me work on this. If I have an idea for your driver do I open a new issue? You might already be working on what I am thinking though. I was thinking that it would be great if this worked with the wireless USB dongle instead of just bluetooth. Let me know what you think.

@atar-axis atar-axis mentioned this pull request Apr 8, 2019
@atar-axis
Copy link
Owner

atar-axis commented Apr 8, 2019

Merged by 769429b
Congratulations!

This pull request was closed.
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