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

Use toolbar to select channel #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hubertmis
Copy link
Collaborator

No description provided.

@hubertmis hubertmis added the enhancement New feature or request label Feb 20, 2019
@stig-bjorlykke
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

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
Copy link
Collaborator Author

hubertmis commented Feb 20, 2019

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

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

@stig-bjorlykke
Copy link
Collaborator

@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
Copy link
Collaborator Author

@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
Copy link
Member

LuDuda commented Feb 25, 2019

@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
Copy link
Collaborator Author

@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
Copy link
Member

LuDuda commented Feb 25, 2019

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

@e-rk
Copy link
Collaborator

e-rk commented Feb 25, 2019

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
Copy link
Collaborator

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.

Copy link
Collaborator

@e-rk e-rk left a comment

Choose a reason for hiding this comment

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

Thank you for sending improvements once more.
One comment below.

self.channel = int(payload)
while self.running.is_set() and not self.setup_done.is_set():
time.sleep(0.1)
self.serial_queue.put(b'channel ' + payload)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be useful to move this code to a separate method to make it accessible to programs using this extcap as a module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Will do

@hubertmis hubertmis force-pushed the pr/use-toolbar-to-modify-channel branch from 0591f21 to 963e2a2 Compare February 25, 2019 21:24
Copy link
Collaborator

@e-rk e-rk left a comment

Choose a reason for hiding this comment

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

I am approving this with just one more question below.

@@ -151,6 +151,17 @@ def correct_time(self, sniffer_timestamp):

return sniffer_timestamp + overflow_count * self.TIMER_MAX

def update_channel(self, channel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think that the name of this method feels a bit off. Wouldn't set_channel sound more appropriate here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not a fan of current name either. set_channel was my first idea, but it sounds like a typical setter. This method is more than a setter - it is used to update the channel value after the channel was set during initialization. That's how I came up with update. Do you think that set_channel would be better anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, IMO set_channel would sound better. I don't think that it requires a special name just because it changes previously initialized value.
One could also use @property decorator, but I think it would make it less readable here.

@e-rk
Copy link
Collaborator

e-rk commented Mar 5, 2019

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

Copy link
Member

@LuDuda LuDuda left a comment

Choose a reason for hiding this comment

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

Thanks for contribution one more time! The code looks great!

I verified that on Linux it works like a charm.

On Windows, however, I would say that it works completely unpredictable. Sometimes it chooses the channel from the toolbar, sometimes from the interface settings. Sometimes it refreshes the select from in the toolbar when the channel is chosen from the interface, but sometimes it locks on previous one e.g. 13 while it receives frames on channel e.g. 18. The user experience is very weak.

I would say that we either try to fix this (maybe add some sleeps 😄)) or remove the interface setting.

Anyhow, please remember to add documentation about a new way of using a sniffer (with toolbar) - in this or the subsequent PR.

@e-rk
Copy link
Collaborator

e-rk commented Mar 5, 2019

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

e-rk commented Mar 5, 2019

@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.

@e-rk e-rk mentioned this pull request Mar 5, 2019
@hubertmis
Copy link
Collaborator Author

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

@e-rk
Copy link
Collaborator

e-rk commented Mar 5, 2019

@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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@stig-bjorlykke
Copy link
Collaborator

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
Copy link
Member

LuDuda commented Mar 8, 2019

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
Copy link
Collaborator

e-rk commented Mar 8, 2019

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
Copy link
Collaborator

stig-bjorlykke commented Mar 8, 2019

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
Copy link
Collaborator

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
Copy link
Collaborator

e-rk commented Mar 12, 2019

@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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants