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

safety check #107

Closed
wants to merge 1 commit into from
Closed

safety check #107

wants to merge 1 commit into from

Conversation

3hhh
Copy link
Contributor

@3hhh 3hhh commented Oct 21, 2023

Check on edrumulus startup, if the ADCs roughly detect the expected 1,65V from the voltage divider circuits. If not, abort and inform the user via serial console about the issue.

The control port is skipped.

This check may may detect:

  • incorrect analog circuits
  • broken microcontroller ADCs
  • pads drawing current
  • accidental shorts

Check on edrumulus startup, if the ADCs roughly detect the expected
1,65V from the voltage divider circuits. If not, abort and inform
the user via serial console about the issue.

The control port is skipped.

This check may may detect:
- incorrect analog circuits
- broken microcontroller ADCs
- pads drawing current
- accidental shorts
@3hhh 3hhh changed the title safety check added safety check Oct 21, 2023
@corrados
Copy link
Owner

That's a good idea to inform the Edrumulus user about a possible DC offset issue.

I have decided to implement this a bit different from your solution. I'll do this check periodically when also the sampling rate check is performed. Then I can detect if, e.g., an input is short cut or during operation the front end has changed (a resistor has no contact after vibration). Also, I have the advantage that I know which input is the control input (which you do not know in the setup function).

If the DC offset is too large, the board LED will flash quickly, indicating clearly that there is something wrong with Edrumulus (note that the board LED will now also flash if the sampling rate is too low which I have implemented some days ago). I decided not to abort the ADC read since even if the DC offset is large, it may be possible to operate Edrumulus almost normal and I do not want to disable Edrumulus completely at some undefined time (maybe during a song with your band...).

This is my commit: 621f3a5

@3hhh
Copy link
Contributor Author

3hhh commented Oct 26, 2023

Sounds reasonable to me.

One of the main points why I wanted to abort reading the ADC was in order to not break the hardware though. As you know it happened to me that the voltage divider was not connected to either 3V3 or GND (due to the "mind the gap" issue) resulting in the piezo with negative voltages to be directly connected to the ADC. That check would've detected it, but as long as the ADC continues reading, the microcontroller is likely to die anyway.

Maybe some sort of global performance mode setting to always attempt to go on with the default being the more safe practice mode would help here. Or simply make the bounds themselves configurable.

If not, I'll likely have to patch your lower bound for my setup as I still have at least one pad running at only slightly above 1V since it's still trying to draw more current than it can get.

Also, I'd suggest to print the offending pins to serial out to help debug hardware issues.

@corrados
Copy link
Owner

That check would've detected it, but as long as the ADC continues reading, the microcontroller is likely to die anyway.

The question is if the ADC died or the multiplexer.

Also, I guess it would have made no difference if the ADC was turned off or on. I assume that your Teensy would have been broken anyway. You have to start the ADC to detect that the DC offset is incorrect and during that time it would have died anyway, I assume.

If not, I'll likely have to patch your lower bound for my setup as I still have at least one pad running at only slightly above 1V since it's still trying to draw more current than it can get.

My bound is set to accept a waste of 1 bit, i.e., half the available maximum dynamic. For a 12 bit ADC, the allowed range is from 1024 to 3072 which should be large enough for your use case.

Also, I'd suggest to print the offending pins to serial out to help debug hardware issues.

The ESP32 cannot use MIDI and serial at the same time.

@3hhh
Copy link
Contributor Author

3hhh commented Oct 31, 2023 via email

@corrados
Copy link
Owner

corrados commented Nov 3, 2023

FYI, I just implemented a new feature on a Git side branch which will give you the information about the problematic pad (input) in the Edrumulus GUI:

grafik

It uses the normal MIDI transfer and therefore, it works for the Teensy as well as for the ESP32.

@3hhh
Copy link
Contributor Author

3hhh commented Nov 3, 2023

That looks like a very good way to report errors. Maybe print them in red though?

I look forward to see that in the main branch! :-)

@corrados
Copy link
Owner

corrados commented Nov 3, 2023

That looks like a very good way to report errors. Maybe print them in red though?

I think this is not necessary since the text is in capital letters and it is blinking:

Bildschirmaufzeichnung.vom.03.11.2023.17.40.29.webm

@3hhh
Copy link
Contributor Author

3hhh commented Nov 4, 2023

I think this is not necessary since the text is in capital letters and it is blinking:

Nice! I agree.

I however still think it would be safer to not read from the ADC on critical errors (i.e. wrong voltages). I'm also unsure about the "what happens if it's during a gig?" argument as both edrumulus refusing to work as well as the ADC breaking will eventually interrupt the gig. And the latter is not even fixable without new hardware.

@corrados
Copy link
Owner

corrados commented Nov 4, 2023

I look forward to see that in the main branch! :-)

It's on main branch now.

I however still think it would be safer to not read from the ADC on critical errors (i.e. wrong voltages).

As already written above in one of my posts, I'm not sure that it would have made any difference if the ADC was on or off. I assume that your Teensy would have died in any case caused by the incorrect voltage.

@3hhh 3hhh closed this Nov 5, 2023
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.

2 participants