fix: Segmentation and out-of-range errors. #5
Conversation
|
Thanks for splitting out the changes. What are the steps to reproduce the problems you were seeing that these changes address? |
|
Hi Elaine, Regarding what I refer to as the "out-of-range" error, in your project on the main branch, you can simply edit the c2dv.cfg file and set COUNT=2 in the CHANNELS section. After making this change, launch the scope application. You’ll notice the following error appears: In my view, this happens because, in ScopeController, the line: As for the segmentation faults, I couldn’t reproduce the errors directly from your code, since they were triggered by the code I wrote. However, some issues arose when the Channel connection callbacks attempted to modify parameter values (to display 'Connecting' or 'Connected' states), which led to recursive calls. For now, I haven’t modified my first pull request, as it’s difficult to split the code. I suggest we address this current pull request first, and then I’ll update the first one based on the decisions we make. |
| self.lastArrays = arraysReceived | ||
| self.arrays = np.append(self.arrays, n)[-10:] | ||
|
|
||
| # Try to disconnect signal to prevent recursive calls of callback and then SegFaults. |
There was a problem hiding this comment.
Thanks for finding that c2dataviewer wasn't safely updating the parameter tree. It looks like pyqtgraph already has a builtin way to prevent recursive calls to the tree change callback. Can you try using treeChangeBlocker in these cases instead of disconnecting and reconnecting the callback. They also have blockTreeChangeSignal/unblockTreeChangeSignal methods, but using treeChangeBlocker is preferable
| """ | ||
|
|
||
| DEFAULT_MINIMUM_CHANNELS_NUMBER : int = 4 | ||
| DEFAULT_MINIMUM_WAVEFORMS_NUMBER : int = 4 |
There was a problem hiding this comment.
The DEFAULT_*_WAVEFORMS_NUMBER variables appear to be unused, can you remove them?
Also, since the minimum and maximum waveform number doesn't change and "4" is the default number, not the maximum, consider having instead:
MIN_NCHANNELS = 1
MAX_NCHANNELS = 10
DEFAULT_NCHANNELS = 4
| ''' | ||
| QtCore.QTimer.singleShot(0, instruction) | ||
|
|
||
| def muted(self, instruction : Callable[[], None]) -> None : |
There was a problem hiding this comment.
probably don't need this since pyqtgraph has a builtin way of blocking parameter tree change signals
| except pva.PvaException: | ||
| #error because too many asyncGet calls at once. Ignore | ||
| pass | ||
| logging.getLogger().exception('Failed to poll with PollStrategy.') |
There was a problem hiding this comment.
Changing this to a debug message, it isn't an error if it gets to this point
| self.ctx.channel.setConnectionCallback(None) | ||
| except PvaException: | ||
| pass | ||
| logging.getLogger().exception('Failed to stop Monitor.') |
There was a problem hiding this comment.
Change this to a debug message, it's not an error at this point.
| else: | ||
| yield sep.join(kprefixs + [k]), v | ||
|
|
||
| def execute_later(instruction : Callable[[], None]) -> None : |
There was a problem hiding this comment.
I don't see this used, please remove
| :param number: The new number of sections that should be displayed. | ||
| ''' | ||
| if number > 10 or number < 1 or type(number) is not int: |
There was a problem hiding this comment.
use MIN/MAX constants
| if previous_number > number: | ||
| self.channels = self.channels[: number] | ||
| while previous_number > number: | ||
| self.muted(lambda : self.parameters.child('Channel %s' % previous_number).remove()) |
There was a problem hiding this comment.
instead of turning on/off signals for each call, maybe just turn them off for the entire function after checking the number range.
| previous_number -= 1 | ||
| else: | ||
| while previous_number < number: | ||
| self.muted(lambda : self.parameters.insertChild(self.parameters.child('Statistics'), Configure.new_channel(previous_number + 1, self.color_pattern[previous_number], ['None'], 0))) |
There was a problem hiding this comment.
Can you add a comment in the code explaining what this part is doing, I think it is a little tricky since it depends on Statistics being right after the Channel sections.
| self.muted(lambda : self.parameters.child("Acquisition").child("Start").setValue(0)) | ||
| # Stop data source | ||
| self.model.stop() | ||
|
|
There was a problem hiding this comment.
Should add the new parameter for channel count the serialize function in ScopeController (this is further down in this file)
2 participants
ScopeController.__init__: The number of channels is now stored in theParameterobject via the config and retrieved byScopeControllerat this point. This avoids errors when the user uses the.cfgfile to configure fewer than four channels (sinceConfigureonly creates twoChannels iparameters; when the controller tries to access the third, for example duringupdate_fdr, the parameter is not found).ScopeController.execute_later: Implementation of a function that waits for the call stack to be emptied before executing an instruction. This function addresses segmentation issues when different threads attempt to modify parameters, especially when channels connect and request updates of the'PV Status'parameter.ScopeController.muted: Addition of a function that allows executing an instruction after disconnection and before reconnection of the parameter-change signal (useful for changing a parameter without triggering the callback function).ScopeController.set_channel_number: Implementation of a feature that allows changing the number of channels displayed on screen. This implementation addresses the following error: when the user specifies more fields on the command line than the number of channels requested in the.cfgfile, an error is raised. The function therefore allows the user to modify the number of acquisition channels and is also called in the previously mentioned case to avoid the error.Use of
ScopeController.mutedto control parameter changes originating from the code.ScopeController.parameter_change: Removal of exception handling in theif childName == "Acquisition.PV"block since error handling is already implemented insideScopeController.update_fdr.ScopeControllerBase.update_status: Disconnection of the parameter-change signal before updating the statistics parameters, and reconnection afterward. On the first call of this function, the signal is normally not yet connected; thetry-exceptstructure allows handling the exception raised when Python tries to disconnect the signal while it is not yet connected.MonitorStrategy.stop: Removal of an unnecessary line that was raising an error.Configure.new_channel: The code generating the dictionary is moved into a separate function that can be called byScopeController.Configure.parse: Change in the order of function calls:assemble_channelmust be called beforeassemble_acquisitionso that the number of channels written in'Acquisition'is the effective numbers.