Skip to content

Tie lifetime of oni.Context to that of ContextTask #409

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

Merged
merged 2 commits into from
Feb 17, 2025
Merged

Tie lifetime of oni.Context to that of ContextTask #409

merged 2 commits into from
Feb 17, 2025

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Feb 12, 2025

Hardware Tests

I tried to put this throgh the ringer

Test Desc Result
Breakout only Stated with breakout only, quickly hitting refresh during acqusition etc. ✔️
Breakout , NP2.0e Added np2.0e headstage, which reliese on passthrough configuration, quickly hitting refresh during acquisition etc. ✔️
Breakout, NP2.0, Rhs2116 headstage A Rhs2116 headstage, which uses a standard port but needs to issue a second reset after power up: ✔️
Breakout, NP2.0, Rhs2116 headstage with auto voltage Same as last but do auto voltage discovery on rhs2116 headstage ✔️

- Instead of disposing and recreating oni.Context when a reset occurs, issue a oni.Context.Refresh() to existing object
@jonnew jonnew requested a review from aacuevas February 12, 2025 17:40
Copy link
Collaborator

@aacuevas aacuevas left a comment

Choose a reason for hiding this comment

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

Seems to be working with no apparent downside indeed.

- Make ContextTask.ctx readonly per @glopesdev's
  comments
- Bump package version
@jonnew
Copy link
Member Author

jonnew commented Feb 13, 2025

Looks like there might be an issue with Miniscope documented by @ChucklesOnGitHub

image

the red lines are indications of issues #406, #401, #408. So these changes might fix that, but the miniscope is now having issues. I will try to reproduce this.

@jonnew
Copy link
Member Author

jonnew commented Feb 17, 2025

@ChucklesOnGitHub I am unable to reproduce this issue. This is the workflow im using:

image

Note that miniscope voltage autodiscovery is being used. No issues running this and capturing analog data from breakout and image data from miniscope.

Can you tell us exactly what error you're encountering? You can right click and copy the message at the bottom left of the bonsai editor when an exception occurs to get a detailed log.

@ChucklesOnGitHub
Copy link
Member

For me, the issue was present using a workflow with only the breakout board:
image

Only in test 2 did the issue appear when using the miniscope, all other miniscope tests (4,6,8 in the table above) were fine.
image

@jonnew
Copy link
Member Author

jonnew commented Feb 17, 2025

Hi @ChucklesOnGitHub, I need clarification about the word "issue".

From the table, I think "issue" refers to the large issue that this PR is attempting to solve. Namely, that in some cases, after stopping the bonsai workflow, the hardware remains engaged as if it was acquiring data instead of stopping normally. Lets call this issue 1. The second issue that I see, is that with this PR applied, there is another secondary issue that appears which is a "Failure to write stream or register" exception. This is a problem, but one that is actually in line with normal operation and some communication issue happening. Let's call this issue 2.

Is this assessment correct? If so, I am not able to reproduce issue 2. In order to figure out if its being created as a result of this PR, i need to know where its coming from. What is the Node in the workflow that produces the exception and if you can copy to the extended text of the exception that would be helpful.

@jonnew
Copy link
Member Author

jonnew commented Feb 17, 2025

OK, so @ChucklesOnGitHub and I met out of context to clarify the situation. It turns out that there a couple of separate issues.

  • First of all, Issue 2 in above definition appears to be read herring. Upon revisiting PR code, the miniscope is OK, or at least as OK as the auto-voltage discovery will allow given its inherent limitations

  • Issue 1 is also solved with this PR

  • A final issue (Hardware stays engaged for 30 seconds after workflow is stopped #401) is that even when stopping the workflow did shutdown the hardware, sometimes this took up to 30 seconds after the workflow was stopped to occur. This was a result of something that is a problem from the users perspective, but actually is not a logical error or but in the code. It occurs because @ChucklesOnGitHub downloaded this example workflow and removed the miniscope to test the breakout in isolation. In this case, the only device that was producing data was the heartbeat. Further, in that workflow, which must have been produced using a old version of the library, the heartbeat rate is set to to 10 Hz

    image

    It should be hardcoded to 100 (Hardcode ConfigureHeartbeat to enabled in ConfigureBreakoutBoard #359). Combined with the large ReadSize that was originally required for the Miniscope, the read buffer took very long to fill in order to allow the read_frame() call to return and hardware shut down to proceed. I'm going to address this separately in Hardware stays engaged for 30 seconds after workflow is stopped #401.

@jonnew jonnew merged commit bdafa9b into main Feb 17, 2025
8 checks passed
@jonnew jonnew deleted the issue-407 branch February 17, 2025 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ContextTask.Reset() should call oni.Context.Refresh() instead of deleting and reopening context
4 participants