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 context manager/monad-like wrapper #6

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

tmheath
Copy link

@tmheath tmheath commented Jan 30, 2024

I've added a wrapping class to minimize downtime of CANape from the user during program execution while using the API. Documentation probably needs to be better fleshed out, I think a couple variables could be better named and tests should probably be added. I'm committing before leaving work for the day since I know I'm not going to have the time to finish every single thing.

This does not fix, but does alleviate #4 Canape Buttons Blocked.

Final note is I had no idea what a pre-commit hook was until now, I did all the manual checks you mentioned, I don't know if that does different, I don't understand how I'm even supposed to work with that. I figure earlier feedback would be better than fully finishing every single detail and than having to change everything.

Thanks for consideration/time

@tmheath
Copy link
Author

tmheath commented Jan 31, 2024

I've finished the main edit pending review.
All but one test case was skipped, I have not
viewed them yet. I'm about to switch tasks
so I'm asking this before I look. Pytest told
me it was skipping because couldn't find canape
or something. Would you consider it good if
I added a secondary suite of mocked tests
so that any of the code can actually run with
full coverage without needing to worry about
the external environment? I have done this on
one of our own projects and don't have a problem
doing it here but it does add a lot and it will be sitting
there to add to maintenance overhead. I'm asking
because I'm not likely to permanently be working
on this project so I don't want to add work that
I'm never going to touch myself.

@zariiii9003
Copy link
Owner

I like the idea of connecting to CANape and immediately releasing it. But i cannot get it to work with more recent versions.
Even with a pure C++ implementation, if i try to connect to an open CANape instance, i get AEC_WRITE_CMD: Error writing data to memory mapped file.

@tmheath
Copy link
Author

tmheath commented Feb 4, 2024

@zariiii9003
I'll test it deliberately by hand Monday, I thought that I had connected to a running instance of CANape 15.
The reason that I haven't updated those docs is because they're a bunch of warning about methods not being
found in the dll so I wasn't sure if it absolutely was supported or not.

@zariiii9003
Copy link
Owner

I don't know about 15, but i had no success with 20 and 21.
Here's the C++ code, that i used:

#include <windows.h>   // for WINAPI in canapapi.h
#include <iostream>
#include "CANapAPI.h"

int main(int argc, char const *argv[])
{
    TAsap3Hdl gHdl = NULL;  // Your Asap3 Handle
    TModulHdl gModule;      // Your Module/Device Handle
    unsigned short gTaskId; // a global task id

    gHdl = NULL;
    // Asap3Init Parameters
    unsigned long responseTimeout = 0;
    char *workingDir = NULL;
    unsigned long fifoSize = 2000;
    BOOL debugMode = FALSE;

    BOOL res = Asap3Init(&gHdl, responseTimeout, workingDir, fifoSize, debugMode);
    if (!res) {
        unsigned short errCode = Asap3GetLastError(gHdl);
        char* errMsg = NULL;
        Asap3ErrorText(gHdl, errCode, &errMsg);
        std::cout << errCode << " " << errMsg << std::endl;
    }
    return 0;
}

@tmheath
Copy link
Author

tmheath commented Feb 4, 2024

In my testing I had a working dir set to a dummy dir I created at the root of my user profile and was passing debug_mode true. I was using a much larger fifo size but I doubt that matters. I had been using the dll from go and then rust manually trying to get, and eventually succeeding everything working. Then I checked with your library and it seemed like 15 was wanting to work.

AEC_WRITE_CMD: Error writing data to memory mapped file.

I was getting that error a couple of the times that I did it manually, I don't remember why right now.
I'll look through things, test things, and get back to you Monday night at the latest. I'm not sure if I'll be able to
rediscover that error.

@tmheath
Copy link
Author

tmheath commented Feb 5, 2024

I read through the official docs, to me they definitely implied connecting to an already open instance, but yeah; behavior is the same. I was able to open in modal_mode (undefined in the official documentation for CANape 15), and only some of the buttons are hidden in the toolbar, I expect scripting and buttons to remain usable. I haven't exhaustively tested everything but CANape 15 does seem to be supported insofar as the generic forms of the functions found in the api aren't defined.

This library is usable and I will be going forward using this for our purposes, at least for now. This PR is definitely less useful without the potential to connect to an open instance. It might still be useful for someone but that's more specific and I'm not sure it's good to have in the library, especially considering how simple the code is. Should I just close this PR as the feature isn't useful enough to warrant the bloat or should I flesh this out and try getting this pushed?

@tmheath
Copy link
Author

tmheath commented Feb 5, 2024

Correction

Python setting interactive mode to true allows use of all buttons again in the toolbar.
This PR retains it's usefulness more with slight modification to instead pass through the
connection without managing the close/open status and might be desirable to group
actions together without really effecting the end user so bad. I believe it would better
be to modify the PR to do that, but at your disgression it still may be better to not add
the PR.

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