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

Added sounds #1032

Merged
merged 22 commits into from
Jun 8, 2020
Merged

Added sounds #1032

merged 22 commits into from
Jun 8, 2020

Conversation

gmmanonymus111
Copy link
Contributor

  • This PR adds sounds to various events, like a job finished, there was an error, or when machine got disconnected. Sounds can be set to anything the user wants, and can be muted if not needed.
  • Fixed $$ response was not visible on Nighty theme.
  • Updated Hungarian translation.
  • Added the possibility to use 2-decimal precision while setting offsets during image import.

This PR should be up-to-date with the latest changes (as of 2020.06.03).
If there are any issues, let me know.

@arkypita
Copy link
Owner

arkypita commented Jun 4, 2020

It "sound" awesome :-)

- use of enum instead of event id numbers
- cleanup PlaySound function and protect against missing file or unreadable file issues
- solve some problems due multiple call location of PlaySound(disconnect)
@arkypita
Copy link
Owner

arkypita commented Jun 4, 2020

I had to make some changes to your code:

First of all an aesthetic modification with the use of an enum instead of int to identify eventId and the cleanup of the PlaySound function.

Second there was an issue because PlaySound(disconnect) was called from too much point that cause for example a double call of PlaySound(disconnect) or a call to PlaySound(disconnect) when closing LaserGRBL even if already disconnected.

Also connect was called not in the right place.

I think that the right place where to call PlaySound(connect/disconnect) is inside SetStatus function that is able to discriminate and discretize state changes.

However I have some doubt on SoundPlayer class usage:
https://stackoverflow.com/questions/4107886/soundplayer-causing-memory-leaks

I should do some test to be sure that there are no memory leak and no issue with this class.

@gmmanonymus111
Copy link
Contributor Author

Thanks for the cleanup! Next time I'll try to do that the first time.
For the event playing location, I wasn't really sure where to call them. That seemed the best place.
SoundPlayer weren't produced any issues for me, tried with a full-invalid nc file (there were no valid GCode in them, just to test SoundPlayer with Warning (non-fatal :P) sounds), and my memory usage was a maximum of 25MB. The file had 1515 lines. But this is only my test, with a decent laptop on Win10. Might try it with a virtual XP to make sure, as it maybe uses older code, where the leak was an issue.

@arkypita
Copy link
Owner

arkypita commented Jun 5, 2020

The memory leak problem I'm talking about is not easily measurable because it is a cumulative effect of a few KB.

The SoundPlayer object allocates unmanaged system resources (resources not included in the automatic memory management/cleanup done by the .net framework) which therefore are not released unless you call the "Dispose()" method of the SoundPlayer object itself.

Since you create a new SoundPlayer object in your code whenever you want to play a sound, and you don't release it with the Dispose(), it is conceivable that there is this cumulative effect.

I'm not sure how to solve this problem, however, because I doubt that calling dispose immediately after "Play" leads to the interruption of the sound, and calling it later - when the sound is finished - is not possible because it is not known when the sound ends (play() is asynchronous).

It is not possible to call PlaySync() because this would block the thread from which the call comes (the thread that takes care of streaming the gcode, for example) for the time necessary to reproduce the sound.

Finally I have the doubt that if the gcode code is received as full of errors from grbl (for example a third party file that contains gcode not implemented by grbl) the "PlaySound (warning)" is executed in a burst a lot of times sending in crisis the system (each Play() call is executed in its own thread, so thousand of thread could be created in this situation).

@arkypita
Copy link
Owner

arkypita commented Jun 5, 2020

Finally I have the doubt that if the gcode code is received as full of errors from grbl (for example a third party file that contains gcode not implemented by grbl) the "PlaySound (warning)" is executed in a burst a lot of times sending in crisis the system.

Especially for this last detail you should do some specific tests and check how it behaves, possibly inserting "anti stress" methods that skip the execution of multiple calls too tight at the playsound.

@arkypita arkypita merged commit bbbdd3b into arkypita:master Jun 8, 2020
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.

None yet

2 participants