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

Add Feature wireless ad hoc network mode #41

Merged
merged 21 commits into from
Jun 17, 2019

Conversation

Laymer
Copy link
Contributor

@Laymer Laymer commented Oct 17, 2018

EDIT : prefixes have been added

Provide possibility to setup ad hoc wireless networks by providing arguments in the grisp.ini configuration file. The parameters are :

  • wlan_mode : if set to "adhoc", the ifconfig command will be given adhoc mode as operation mode
  • wlan_adhocname : SSID of the ad hoc network
  • wlan_channel : the operating channel in the 2.4GHz band
  • wlan_ip_netmask : subnet mask for the assigned IP address
  • wlan_ip_self : assigned IP address used for wireless configurations
  • wlan_hostname : hostname used for wireless configurations

EDIT N°2

The standard ip_self and hostname attributes have been left independent from the wlan_-prefixed parameters, so that the standard configuration is identical and if a Pmod Ethernet module is added in the future, these attributes could be used for that purpose for example.

Laymer and others added 10 commits July 22, 2018 20:40
Provide possibility to setup ad hoc wireless networks by providing arguments in the grisp.ini configuration file.  The parameters are :    
- wlanmode : if set to "adhoc", the ifconfig command will be given adhoc mode as operation mode 
- adhocname : SSID of the ad hoc network 
- channel : the operating channel in the 2.4GHz band 
- ip_netmask : subnet mask for the assigned IP address
This reverts commit 6dacfbb.
This reverts commit 684f669.
Copy link
Member

@eproxus eproxus left a comment

Choose a reason for hiding this comment

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

Indentation uses mixed tabs and spaces, please use 4 spaces and no tabs

@eproxus
Copy link
Member

eproxus commented Nov 6, 2018

I'm wondering if it would make sense to prefix the WLAN specific configuration attributes with wlan_ such as wlan_channel etc. in case we add other similar attributes for other network types later...

@Laymer
Copy link
Contributor Author

Laymer commented Nov 7, 2018

@eproxus Thank you so much for taking the time to review the PR ! I will refactor the code to match the RCs and update ASAP. Regarding the wlan_ prefix, I am not 100% sure but I think that the channel attribute for example is only applicable for wireless cloned interfaces. But I think that would be better anyway for the reason you mentioned, ( and would look cleaner in my opinion ) so I will add the prefix to all the attributes.

@peerst
Copy link
Contributor

peerst commented Nov 7, 2018

Agree on the prefix

The attributes now designate explicitly that they relate to a wireless setup.
Occurrences of the "tab" character have been replaced by 4 spaces.
@eproxus
Copy link
Member

eproxus commented Nov 12, 2018

@Laymer sorry to be a bother, but could you revert the white space changes on lines which you haven't modified? That would make the diff much smaller and easier to review

@Laymer
Copy link
Contributor Author

Laymer commented Nov 12, 2018

@eproxus Of course, it is no bother at all. I have just a quick question about the original file. Is it possible that there are tab indentation characters at some places in the erl_main.c file and some where there are only 2 spaces?

@eproxus
Copy link
Member

eproxus commented Nov 12, 2018

@Laymer May well be. We've had problems of mixed tabs and spaces before and perhaps not everything is cleaned out. Sorry for the mess

@Laymer
Copy link
Contributor Author

Laymer commented Nov 12, 2018

@eproxus No worries, I was just asking to make sure. I will push a quickfix in no time.

@Laymer
Copy link
Contributor Author

Laymer commented Nov 12, 2018

@eproxus I have reverted all the whitespace changes I could spot, I hope it makes it easier to review the code itself. Please feel free to let me know if there are some additional changes I should make.

@eproxus
Copy link
Member

eproxus commented Nov 13, 2018

@Laymer That looks simple and nice, thank you!

@peerst @sylane Can you review this please?

@Laymer
Copy link
Contributor Author

Laymer commented Nov 13, 2018

@eproxus Glad if I can help ! Thank you for taking the time to review all.

@peerst
Copy link
Contributor

peerst commented Nov 13, 2018

Why is there a wlan_hostame in addition to the normal hostname and what is their relationship?

@Laymer
Copy link
Contributor Author

Laymer commented Nov 13, 2018

@peerst I have tried to comply with the following :

I'm wondering if it would make sense to prefix the WLAN specific configuration attributes with wlan_ such as wlan_channel etc. in case we add other similar attributes for other network types later...

But the original hostname has not been modified at all so I think that there is no relationship between them. I also imagined that maybe it would be possible in the future to use the Pmod Ethernet module, so two different hostnames could coexist. But this was a pure assumption and I have left the default hostname just in case removing it would introduce conflicts.

@peerst
Copy link
Contributor

peerst commented Nov 13, 2018

But wouldn’t they be set to the same name 100% of the uses? If I have a machine with multiple network interfaces it still has one hostname. Or is there a reason to have a different hostname from the wlan_hostname?

Wouldn’t it make sense to just use the hostname for wlan?

@peerst
Copy link
Contributor

peerst commented Nov 13, 2018

We had a offline discussion and decided to just use hostname for both since we couldn’t identify a use case for different hostname and wlan_hostname. Just another know that can be set wrongly

Copy link
Contributor

@peerst peerst left a comment

Choose a reason for hiding this comment

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

Everything fine, I think we should merge this

@peerst peerst merged commit ed0b868 into grisp:master Jun 17, 2019
@eproxus
Copy link
Member

eproxus commented Jun 18, 2019

This feature needs documentation, either in the README or on the wiki.

@Laymer
Copy link
Contributor Author

Laymer commented Jun 18, 2019

@eproxus This is something I will definitely work on, as it is a hard requirement for me to make it usable and configurable. Now that this PR is merged I could open another one for the docs and write down a wiki page for it if that is okay for GRiSP? 🙂

@eproxus
Copy link
Member

eproxus commented Jun 18, 2019

@Laymer Sounds good!

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.

None yet

4 participants