Use toolbar to select channel

[stig-bjorlykke]

One issue with having the configuration for Channel both as a interface option (configured with the cogwheel) and in the toolbar is that when you start capturing the value you set in the toolbar is not respected. The intention with the toolbar is that the values you set is always used when starting a capture, not other configurations or default values.

If I stop the capture and start again, or do a restart, I would expect the value in the toolbar to be used. Currently it's always reverted back to the interface options value.

This can be solved by reading and using the value from control_in before getting the CTRL_CMD_INITIALIZED signal from the toolbar.

[hubertmis]

@stig-bjorlykke , thanks for this info. Indeed I have not tested it with restarts. Do you recommend to remove interface option or use control_in on restarts?

[stig-bjorlykke]

I will recommend to remove the interface option and only use the setting from toolbar. The user will bring up the toolbar and configure the channel before starting a capture, or while capturing, and the value will stay until changed by the user. This way the value in the toolbar is always correct.

One disadvantage is that the last used channel value is not stored, and restarting Wireshark will default back to 11. I don't think this is a big issue.

[hubertmis]

Perhaps I could leave --channel command line option to keep compatibility with automatic tests and remove channel configuration from the --excap-interfaces output.

@LuDuda @e-rk Thoughts?

[hubertmis]

@stig-bjorlykke I think I've managed to keep interface configuration while correctly using toolbar on capture restarts (reading control_in data). I performed some manual tests and seems to work fine. Do you see any problems with this approach?

[stig-bjorlykke]

@hubertmis The toolbar seems to work like expected, but the setting in interface options is ignored so I still think this should be removed. The --channel option can still be supported without the availability in GUI.

[hubertmis]

@stig-bjorlykke How did you test interface options? It works fine on my home Linux machine (arch)

[stig-bjorlykke]

@hubertmis I try to set a value different than 11 and start capturing, then observed that the toolbar was still showing 11 and the packets was from channel 11. In general it's difficult to be able to configure one value from two different places. Who will win when both are different from the default value?

[hubertmis]

@stig-bjorlykke In my setup it worked like following:

1. I set interface to channel 19 and start capture.
2. Capture starts at 19 and automatically sets toolbar value to 19.
3. I change toolbar value to 15. Sniffer changes its channel.
4. I restart sniffing process. The sniffers stays at channel 15 and channel at toolbar is still 15.
5. I change toolbar value to 19 and sniffer changes channel to 19.

It is intuitive for me. However if it does not work on all platforms I'll just remove channel from interface configuration.

[LuDuda]

@hubertmis Thank you for this contribution! Looks great!

I can test this PR on my Windows machine. @hubertmis can you rebase it to the master?

However, if @stig-bjorlykke recommends removing the channel from the interface I would strongly consider doing so. One potential problem with this approach, however, is that we also use this sniffer extcap python script without Wireshark (e.g. in CI of our SDK examples).

@e-rk Thoughts?

[hubertmis]

@LuDuda We can remove channel from Wireshark interface GUI, while maintaining command line option to select the channel if the script is used without Wireshark. What do you think about this approach?

[LuDuda]

@hubertmis you are right, I didn't think that through.

[e-rk]

While I think that adding GUI capabilities like changing the channel during the capture is extremely useful from Wireshark user standpoint, I feel that usability as a module is just as important here. I think that the moment we have to face a question of adding a capability that would only work with the Wireshark GUI, it should be the time to consider separating the sniffer communication logic from the extcap logic. Perhaps this is a bit of an overkill, if the only capability we need is the option to change the capture channel on the fly, however if there are going to be plans to add more GUI elements, some refactoring should be done in my opinion. That being said, as long as any functionality that can be accessed using the Wireshark can be used by other python programs by importing the module, I am OK with the change. However I recall discussion from a while ago about integrating a CI with this project. If we were to use the tshark to test the interoperability of the extcap, not being able to select the channel would be limiting. The CI scheme was not yet fully thought out though.

[stig-bjorlykke]

Using tshark to set channel is a very good argument for keeping the interface option.

Actually I forgot about the feature that if a setting is never changed by the user in the toolbar it will never be sent to the extcap utility (because the toolbar may not be enabled by the user). It's just a bit hard to understand which value is used when starting the capture. Try changing the channel in both the toolbar and interface options, and observe that the toolbar setting wins over the interface option.

I'll have look at how to improve Wireshark to better visualise which value will be used to the user.

[e-rk]

After f2f discussion it was decided to rename the method to set_channel.

[e-rk]

@LuDuda I have experienced this on Windows today as well. In my case the toolbar settings take precedence all the time. I have also found out that the toolbar value can't be changed by the extcap during startup.

I think I have satisfactionary solution to this. When the capture is started with the tshark, the --extcap-control-in and other arguments are not passed to the extcap. In this case we can use the interface setting. In case of the Wireshark we can ignore the interface settings and use toolbar settings exclusively. The extcap should also have the interface options window removed.

[hubertmis]

@e-rk If we expose the interface setting to the user, we should use it. If we decide to not use it when toolbar is enabled, we should not expose interface setting to the user.

[e-rk]

@hubertmis Yes, this is why I proposed removing that window and relying on the toolbar exclusively.

Clarification: By exclusively, I mean when --extcap-control args are passed.

[hubertmis]

@e-rk, Ok I thought tshark needs interface option to use it. Then I'm ok to remove it.

[e-rk]

@hubertmis I forgot that extcap needs to report all options to be able to receive them in tshark. Turns out, the tshark needs the interface options after all, and because there is no clear indication whether the tshark or the Wireshark was used, the behaviour can't be adapted to both. This and the different behaviour on Windows with the toolbar value put this situation in a bind unfortunately.

If I had to choose between both, I would go with the GUI in the end.

Sorry for making false hopes.

[stig-bjorlykke]

If the channel control combination has issues on Windows only then it seems like a Wireshark issue which should be fixed. I'll have a look at this.

[hubertmis]

Do we have any decision here? Should we remove channel option from interface configuration until Wireshark issue is fixed?

[stig-bjorlykke]

I'll vote for keeping the channel option, based on the knowledge that tshark will be used for testing. It's reported to work correctly on Linux and macOS, and if it's an issue on Windows I'm pretty sure this is a Wireshark issue. Hopefully I'll be able to check this during the weekend.

[LuDuda]

Due to the limited time we have before the first release of the sniffer (let's say ver 0.5.0), I would vote for postponing of merge this PR to the next production release, unfortunately. I think we should dedicate more time to make sure Windows is well supported as well.

[e-rk]

As much as I would like to have the channel selection toolbar, I have to change my mind and admit that I have some concerns right now. The interface option approach is proven to cause no problems across platforms and with the tshark.

[stig-bjorlykke]

Another approach here is to discard the channel fetched from the toolbar during start capture (before receiving CTRL_CMD_INITIALIZED) and alwayst start with the value from Interface Options. The user will then always start with the channel set in Interface Options, but will be able to change from the toolbar while capturing.

This should be easy to test by discarding the "channel set" in control_reader() if not initialized.

It's not an ideally solution (as described in my first comment) but it's explainable and should work equally on all platforms.

[stig-bjorlykke]

I have tested on Windows and I find it to work exactly like it does on Linux and macOS.

Some information how this actually works:

1. The extcap is always started with the interface options as command line parameters, regardless if the user did any changes or not.
2. The toolbar value is only sent to the extcap if it's changed by the user. It's not sent if having a default value or previously received changes from the extcap. This is because the user may not even enable the toolbar.
3. This extcap sniffer has the channel both as interface option and toolbar value. Because of this the sniffer has to decide which value to use if both are provided (i.e. the user has changed the toolbar channel).
4. Starting a capture can be done in several ways and it's not possible to detect if this was done from the interface options dialog (i.e. prefer the interface options channel) or from the menu, toolbar button, keyboard shortcut or double click the interface (i.e. prefer the toolbar channel).
5. The current behavior in this PR is always using the toolbar channel if the user has changed it, and is always using the interface option channel if the user didn't change the toolbar channel. Using tshark will always work because it does not have a toolbar.
6. The toolbar has support for adding a "restore" button to restore all toolbar values to default. This will remove the "changed by the user" flag and can be used in the extcap sniffer to revert back to using the interface options channel. I have earlier identified that Wireshark needs to improve to better visualize if a toolbar option has been changed or not, but this is not solved yet.

Based on this I think the current behavior is the best solution to handle the channel settings. This should of course be documented.

It has been requested a feature to be able to set the toolbar values when starting tshark (like with the interface options), but this request is not fulfilled yet because tshark does not use the control channels.

[e-rk]

@stig-bjorlykke Thank you for investigating this. The first release will be made without the toolbar feature but I will come back to taking care of this once the release is done.

[stig-bjorlykke]

The first release will be made without the toolbar feature but I will come back to taking care of this once the release is done.

Just remember to remove the dummy toolbar before the release because it does not provide any functionality and will confuse users.