MAVProxy icon indicating copy to clipboard operation
MAVProxy copied to clipboard

paramedit: fix for #872 on macOS

Open srmainwaring opened this issue 3 years ago • 9 comments

This PR is to fix issue #872 preventing the parameter editor window from loading on macOS.

The initial issue was a missing import wx_processguard before the wx module import. Resolving this uncovered another issue where the child task required access to the state of the parent process which caused an error when using multiprocessing spawn.

To fix this the additional state required by the child task is passed in the task arguments on initialisation.

A further minor change to checklisteditor.py updates the deprecated wxPython PyGridCellEditor to GridCellEditor.

These changes tested on:

  • macOS Catalina 10.15.7, Python 3.9.1, wxPython 4.1.0
  • Ubuntu 18.04, Python 3.6.9, wxPython 4.1.1
  • Ubuntu 18.04, Python 2.7.17, wxPython 4.1.1

image

There is an unresolved issue where the Create method on the custom GridCellEditor classes is not being called when the 'Value' cell for a parameter is edited directly:

Loaded module paramedit
'GridScrollEditor' object has no attribute '_tc'

Update:

The 'GridScrollEditor' issue is not specific to this change. It occurs when the 'Value' cell is edited directly (rather than via the 'Selection' cell). On a parameter changed event there is a call to set_checked on the Selection cell:

https://github.com/ArduPilot/MAVProxy/blob/54f1a814eed7d48c4e5787ebd2b03fadf13298c6/MAVProxy/modules/mavproxy_paramedit/param_editor_frame.py#L525

which can occur before the Selection cell editor has been activated by an event. A check inside this function will prevent the exception - but the 'Value' and 'Selection' cells still need to be correctly synchronised.

@stephendade this change may allow the condition on the platform.system() to be removed, as windows and macOS are both using spawn.

https://github.com/ArduPilot/MAVProxy/blob/54f1a814eed7d48c4e5787ebd2b03fadf13298c6/MAVProxy/modules/mavproxy_paramedit/param_editor.py#L96-L107

I've tested using multi-processing and threading options on Windows 10 20H2, Python 3.9.2, wxPython 4.1.1 and find that the parameter editor works in both cases, but with the threading option there is an error when exiting MAVProxy:

Exception in thread Thread-2:
Traceback (most recent call last):
  File "C:\Program Files\Python39\lib\threading.py", line 954, in _bootstrap_inner
    self.run()
  File "C:\Program Files\Python39\lib\threading.py", line 892, in run
    self._target(*self._args, **self._kwargs)
  File "c:\users\rhys\documents\code\ardupilot\mavproxy\MAVProxy\modules\mavproxy_paramedit\param_editor.py", line 217,k
    self.app.MainLoop()
  File "C:\Users\rhys\AppData\Roaming\Python\Python39\site-packages\wx\core.py", line 2237, in MainLoop
    rv = wx.PyApp.MainLoop(self)
wx._core.wxAssertionError: C++ assertion "m_count > 0" failed at ..\..\src\common\object.cpp(336) in wxRefCounter::DecRt
Unloaded module paramedit
MANUAL>
Exception in thread Thread-3:
Traceback (most recent call last):
  File "C:\Program Files\Python39\lib\threading.py", line 954, in _bootstrap_inner
    self.run()
  File "c:\users\rhys\documents\code\ardupilot\mavproxy\MAVProxy\modules\mavproxy_paramedit\param_editor.py", line 39, in run
    while not self.event_queue.empty():
  File "C:\Program Files\Python39\lib\multiprocessing\queues.py", line 129, in empty
    return not self._poll()
  File "C:\Program Files\Python39\lib\multiprocessing\connection.py", line 262, in poll
    return self._poll(timeout)
  File "C:\Program Files\Python39\lib\multiprocessing\connection.py", line 333, in _poll
    _winapi.PeekNamedPipe(self._handle)[0] != 0):
BrokenPipeError: [WinError 109] The pipe has been ended

which may be caused by using multi-processing pipes and queues with a threading child task. I'd favour removing the Windows specific code but will take your steer on that.

srmainwaring avatar Mar 03 '21 17:03 srmainwaring

tested on Big Sur python 3.9.1

jmachuca77 avatar Mar 10 '21 06:03 jmachuca77

fails with py2 with wx-gtk4.0

Traceback (most recent call last):
  File "/usr/lib/python2.7/multiprocessing/process.py", line 267, in _bootstrap
    self.run()
  File "/usr/lib/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/param_editor.py", line 200, in child_task
    self.app.frame.set_param_init(mav_param, vehicle_name)
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/param_editor_frame.py", line 198, in set_param_init
    self.redraw_grid(params)
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/param_editor_frame.py", line 314, in redraw_grid
    self.add_new_row(row, paramname, paramvalue)
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/param_editor_frame.py", line 375, in add_new_row
    self.display_list.SetCellEditor(row, PE_OPTION, cle.GridScrollEditor(Range, PE_VALUE, pvalue))
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/checklisteditor.py", line 148, in __init__
    super(GridScrollEditor, self).__init__()
  File "/usr/lib/python2.7/dist-packages/wx-3.0-gtk3/wx/grid.py", line 262, in __init__
    def __init__(self): raise AttributeError, "No constructor defined"
AttributeError: No constructor defined

tridge avatar Mar 15 '21 23:03 tridge

Updated to ensure compatibility with Python2 and rebase on master.

srmainwaring avatar Mar 16 '21 01:03 srmainwaring

Updated to ensure compatibility with Python2 and rebase on master.

it now creates the window, but dies afterwards:

Loaded module paramedit
  File "/usr/lib/python2.7/threading.py", line 774, in __bootstrap
    self.__bootstrap_inner()
  File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/param_editor.py", line 144, in mavlink_message_queue_handler
    traceback.print_stack()
  File "/usr/lib/python2.7/threading.py", line 774, in __bootstrap
    self.__bootstrap_inner()
  File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/param_editor.py", line 144, in mavlink_message_queue_handler
    traceback.print_stack()
  File "/usr/lib/python2.7/threading.py", line 774, in __bootstrap
    self.__bootstrap_inner()
  File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "build/bdist.linux-x86_64/egg/MAVProxy/modules/mavproxy_paramedit/param_editor.py", line 144, in mavlink_message_queue_handler
    traceback.print_stack()

tridge avatar Mar 18 '21 00:03 tridge

Sorry about this @tridge - I missed one of the Python2 wxPython compatibility changes, for some reasons it didn't raise an exception in my linux environment.

I've reworked the PR to have the minimal change to work in macOS and squashed and rebased on master. I'm concerned about the potential impact on windows because threading is used with multiprocessing queues and I'm not certain that's ok. I'll move this to draft until I can get an independent check on that platform.

srmainwaring avatar Mar 19 '21 11:03 srmainwaring

I'll run some tests and reviews over Windows later in the week.

stephendade avatar Mar 21 '21 08:03 stephendade

I've now had a look at this on Windows.

The major issue is that if I open the editor window and then close it, MAVProxy crashes with the following error:

HOLD> Traceback (most recent call last):
  File ".\MAVProxy\mavproxy.py", line 1467, in <module>
    print("Unloading module %s" % m.name)
OSError: [WinError 1] Incorrect function
Exception in thread Thread-3:
Traceback (most recent call last):
  File "C:\Program Files\Python38\lib\threading.py", line 932, in _bootstrap_inner
    self.run()
  File "C:\Users\stephen\AppData\Roaming\Python\Python38\site-packages\mavproxy-1.8.34-py3.8.egg\MAVProxy\modules\mavproxy_paramedit\param_editor.py", line 37, in run
  File "C:\Program Files\Python38\lib\multiprocessing\queues.py", line 123, in empty
    return not self._poll()
  File "C:\Program Files\Python38\lib\multiprocessing\connection.py", line 257, in poll
    return self._poll(timeout)
  File "C:\Program Files\Python38\lib\multiprocessing\connection.py", line 328, in _poll
    _winapi.PeekNamedPipe(self._handle)[0] != 0):
BrokenPipeError: [WinError 109] The pipe has been ended

stephendade avatar Mar 26 '21 04:03 stephendade

@stephendade thank you for taking a look at this. I'll endeavour to replicate and fix.

I have a Windows 10 VM environment which I've set up according to the wiki. Unfortunately I have somewhat mixed results when running it. The default Cygwin install seems to default to Python 3.6 which is no good. I have installed Python 3.9 directly from python.org which I've had more success with (but not in Cygwin) and can run MAVProxy and SITL from a straight cmd terminal, but am wondering whether this is the recommended approach for testing under Windows. Would appreciate any pointers how best to get Windows configured so its dev - standard and can put my testing on a firmer footing.

srmainwaring avatar Mar 29 '21 22:03 srmainwaring

The default Cygwin install seems to default to Python 3.6 which is no good

The Windows MAVProxy is designed to run directly, without Cygwin. Take a look at the Azure pipeline setup (https://github.com/ArduPilot/MAVProxy/blob/master/.azure/azure-pipelines.yml) to see how the dependencies are set up.

but am wondering whether this is the recommended approach for testing under Windows

ArduPilot SITL will auto-detect if there's a MAVProxy exe to use. You'll need to:

  1. run ./windows/MAVProxyWinBuild.bat to build the MAVProxy installer
  2. Run the generated installer
  3. Run SITL and test

stephendade avatar Apr 01 '21 07:04 stephendade