MrYsLab / pymata-aio

This is the second generation PyMata client.
https://github.com/MrYsLab/pymata-aio/wiki
GNU Affero General Public License v3.0
155 stars 51 forks source link

Fixed out of bounds in _report_version #75

Closed dmyersturnbull closed 6 years ago

dmyersturnbull commented 6 years ago

_report_version needs a sysex_data param to prevent an index out of bounds when it's called through PrivateConstants.REPORT_VERSION on line 1329: await self.command_dictionarysysex[0]

MrYsLab commented 6 years ago

@dmyersturnbull Thanks for taking the time to submit this. I am not sure if I understand what the problem is. Report Version is not a sysex command: https://github.com/firmata/protocol/blob/master/protocol.md, and therefore there should not be any sysex data associated with it.

The line that calls the _report_version is line 1418. https://github.com/MrYsLab/pymata-aio/blob/d6d0ef7a97164f37fd8938a2e43ffc24afcf427b/pymata_aio/pymata_core.py#L1418

Do you have an example of a script that generates the index out of bounds error?

Thanks

dmyersturnbull commented 6 years ago

@MrYsLab Thanks for the fast reply. It does seem that I was looking at the wrong place. In Windows 10 and multiple recent versions of pymata, we're getting lots of errors like this:

_analog_mapping_response() missing 1 required positional argument: 'data'
_pixy_data() missing 1 required positional argument: 'data'

We're getting errors for things we're not even (knowingly) using, like pixy and sonar. I guess that would mean that pymata is receiving malformed commands? Updated

MrYsLab commented 6 years ago

@dmyersturnbull Are you seeing this behavior after successfully starting your program, but then reset the program and the issue occurs?

If not, please describe the scenario that causes the issue including what pin mode types are being set by your program. Including the code would also be helpful.

If my description above is accurate, it is due to the fact the Arduino cannot easily be "hardware reset" from software and that the Firmata framing does not lend itself to when the Arduino and client get out of sync. I have a possible working around, called keep_alive that works for some boards. It requires the use of FirmataPlus (my version of Firmata) to be used. After enabling the keep_alive feature, if the Arduino does not receive the auto-generated keep alive signal from pymata-aio within a given period, that Arduino will take advantage of its internal watchdog timer to perform a hardware reset. Unfortunately, the internal watchdog timer is not supported by all Arduino boards. The Uno and Leonardo do, the Mega does not, and I am not sure about the other boards.

What is happening is that if you have enabled analog input for example, and then reset your program, the Arduino does not stop sending analog data unless you physically repower the Arduino. Firmata, on the other hand, assumes that things never go out of sync and therefore does not provide a framing method to handle the unexpected, out of frame data. This results in the exception you are seeing.

MrYsLab commented 6 years ago

Please let me know if you wish to continue discussing this or not. Thanks.

dmyersturnbull commented 6 years ago

@MrYsLab Yes, we're still looking at it and trying to pin down the issue. Strangely, we get these out-of-frame packets even after sending a reset, physically replugging it, and then running. We modified pymata_core to swallow those exceptions---and we get several of them, but our arduino writes seem to work anyway. Do you have an idea what might cause Firmata to get out of sync even after a clean reset and shutdown?

MrYsLab commented 6 years ago

I have not seen the behavior you are describing. Could you please provide some example code so that I can try to recreate the problem here?

MrYsLab commented 6 years ago

I am going to close this pull request since I have not heard back from you. If you are able to provide some code that demonstrates the issue or want to propose a fix, please open up a new issue or pull request. You probably are aware of this already, but if you are integrating a non-asyncio library into your application, it may cause unexpected issues. Anything that may block, such as using time.sleep, or blocking i/o will likely result in unexplained results. So, if you are integrating with a non-asyncio library you might want to use PyMata.

If you have any questions, please feel free to post here, and I will see them. Thanks.

dmyersturnbull commented 6 years ago

@MrYsLab Yes, thanks for your help! We figured it out on Friday: These errors happen if the sampling_interval is set at 10–~15. Setting it higher fixes the problem. Is there somewhere I should make a note of this on the wiki? I can also make an issue and immediately close it (for reference).

MrYsLab commented 6 years ago

@dmyersturnbull Thanks for letting me know. I wonder if this is not specific to your application. I wrote a short test program that toggles an LED every .0001 seconds using PWM and while continuously monitoring an analog input, and I do not see the behavior you are seeing. I also commented out the print statement in the callback function to further aggravate things and still no issues. Here is my test code:

import asyncio

from pymata_aio.pymata_core import PymataCore
from pymata_aio.constants import Constants
# noinspection PyCompatibility

# call back routine for the current potentiometer value
async def my_callback(data):
        z = data
        print(data)

# noinspection PyCompatibility
async def test_sampling_rate(my_board):
    """
    This method will test the Arduino sampling rate when set down to 10 ms.

    Set digital pin 6 as a PWM output connected to LED
    and set its output value to 128.

    Set pin A2 as analog input connected to potentiometer.

    Set the sampling interval to 10 ms.

    Toggle between 0 and 128 every .01 seconds.

    @param my_board: A PymataCore instance
    @return: No Return Value
    """
    # set the pin mode
    await my_board.set_pin_mode(6, Constants.PWM)
    await my_board.set_pin_mode(2, Constants.ANALOG, my_callback, callback_type=Constants.CB_TYPE_ASYNCIO)
    await my_board.set_sampling_interval(10)

    while True:
        try:
            # set the pin to 128
            await my_board.analog_write(6, 256)

            # toggle the led every .0001 seconds
            await asyncio.sleep(.0001)
            await my_board.analog_write(6, 0)
        except KeyboardInterrupt:
            # shutdown
            # await my_board.shutdown()
            pass

# create a PyMataCore instance and complete the initialization with a call to start()
board = PymataCore()
board.start()

# get the event loop, and execute pin6_pwm_128
loop = asyncio.get_event_loop()
try:
    loop.run_until_complete(test_sampling_rate(board))
except KeyboardInterrupt:
    asyncio.ensure_future(board.shutdown())