FIRST-Tech-Challenge / SkyStone

FTC SDK
https://www.firstinspires.org/robotics/ftc/what-is-first-tech-challenge
274 stars 1.04k forks source link

NullPointerException with bulk caching #232

Closed cdbbnnyCode closed 3 years ago

cdbbnnyCode commented 4 years ago

When using BulkCachingMode.AUTO, OpModes crash with the error
java.lang.NullPointerException: Attempt to invoke virtual method 'boolean com.qualcomm.hardware.lynx.LynxModule$BulkData.getDigitalChannelState(int)' on a null object reference
when reading from digital channels.

Windwoes commented 4 years ago

It appears there is a bug in the implementation here. getBulkData() is equipped to handle NACKs from the module by returning placeholder data, but the recordBulkCachingCommandIntent() method ignores the return value of getBulkData(), simply looking at lastBulkData instead.

I haven't tested this, but I believe the issue can be fixed by doing the following: https://github.com/OpenFTC/Extracted-RC/commit/db88b6f9deac5f21535d2580b8f98f18f4481412

realquantumcookie commented 4 years ago

This also happens for encoder readings from the expansion hub. Please fix this issue ASAP...

Windwoes commented 4 years ago

@ToiletCommander Judging from the release schedule as of late, it will probably be a while before this is fixed. As a workaround, you could turn bulk caching off and instead directly interpret the return values from getBulkData() as this will avoid the issue.

realquantumcookie commented 4 years ago

@FROGbots-4634 Thank you for your advice, we have a state tournament next weekend and I have just recently changed our library to do MANUAL Bulk Read(with a data refresh every cycle my library does a cycle), which in theory should boost the performance of everything (especially trajectory follow responsiveness). It would be a pain in the ass for me to change back to no caching and let every wrapped-up sensor class read from BulkData class... Any est. release date I can refer to that you know of?

realquantumcookie commented 4 years ago

I was able to find a way to work around it...And this is for MANUAL MODE ONLY If you are using MANUAL mode, you can add the following code after your clearBulkCache() function.

Before:

for(LynxModule i : allExtensionHubs){
    i.clearBulkCache();
}

After:

for(LynxModule i : allExtensionHubs){
    i.clearBulkCache();
    try {
        Class<LynxModule> LynxModuleClass = LynxModule.class;
        Field lynxModuleField = LynxModuleClass.getDeclaredField("lastBulkData");
        lynxModuleField.setAccessible(true);
        lynxModuleField.set(i,i.getBulkData());
    }catch(NoSuchFieldException|IllegalAccessException e){
        e.printStackTrace();
    }
}

As clearBulkCache() function sets the lastBulkData to null, we can prevent the bug from happening by manually assign a new data to it, this guarentees that lastBulkData is never set to null.

David10238 commented 4 years ago

Fixing it to just use the last bulk data would be good, but I would like to also see a .didLastReadFail method and the ability to easily stop an opmode with a custom message (you might not want to continue auto if it dc'd). stopping with a custom message could also be a good debugging tool to serve the same function as exceptions while being more user friendly. api could just be requestOpModeStop("insert some message here")

NoahAndrews commented 3 years ago

This was closed because it's fixed in version 5.5.