search

hamishcoleman / debian-minimal-builder

Tool for creating minimal installs of debian-based systems
GNU General Public License v2.0
21 stars 11 forks source link

Fix mesh point interface freq set via conf #7

Closed benhylau closed 6 years ago

benhylau commented 6 years ago

While we could simply remove the quotes to address https://github.com/hamishcoleman/debian-minimal-builder/issues/6, this is a more conscious way to parse the freq config. This handles omission of the HT second parameter with the cut -s flag.

darkdrgn2k commented 6 years ago

There is a note in the file that HT has not been implemented yet and it is a "todo". So hacking around this may not be the right approach instead.

Perhaps it be better to add a new field to [mesh] such as 

[mesh]
ht=HT20

The script already loads all the mesh paramters automatically so no need to have to read the newly defined values from the conf. it will just be $conf_ht

Here is something i was thinking

https://github.com/hamishcoleman/debian-minimal-builder/compare/master...darkdrgn2k:meshscript?expand=1

The above script add the following optional parameters to [mesh] ht - [HT20|HT40+|HT40-|NOHT|5MHZ|10MHZ] mcast_rate - mesh_forwarding - [0|1] mesh_rssi_threshold -

benhylau commented 6 years ago

I kept the freq as single conf because the iw man page has both as the value for freq, if I read it correctly. This also makes the unit file backwards compatible so downstream projects' unit files won't need to make changes.

Those are the reasons but I am also not entirely opposed to separating out a ht field. Let's see what @hamishcoleman thinks.

darkdrgn2k commented 6 years ago

The example above makes the new options optional making any "older" unit files work fine.

I'm just not a fan of over-complicated code. and inline cutting up for a variable just so it can be wrapped in quotes seems counter productive.

Whats the point of it in this case anyway?

[edit] Ah you mean you where using freq="xxx ht20" in the past. I don't think it was meant to be used that way.

why not just drop the "" then

hamishcoleman commented 6 years ago

I with @darkdrgn2k on over complicating things. For this case, I think we can just remove the quotes.

The todo items in that file were more around adding additional idempotent iw commands to set the interface mode bits one at a time than expanding the args to the one command - which looks possible to do from the iw docs, but I have not tested it.

So - in the future - once these idempotent options are in place, a config format breaking change could be made to fix it up to force the "freq=" to /only/ contain a frequency.

Another way to look at is that the setting required to communicate with other people is both a frequency and a channel width - but I think there are already other config in that file that is required to be able to communicate.

As for misusing the value to have the HT20 in there - I think I just took a shortcut to do that as a quick setting, so yes, it is 'misusing'

Finally, I'm going to keep pointing it out that you really dont want to use anything other than HT20 on the 2.4Ghz band and using anything else has range and SNR implications for the 5Ghz band too..

benhylau commented 6 years ago

I have made https://github.com/hamishcoleman/debian-minimal-builder/pull/8 to simply remove the quotes.