Add ACIO Manager - [merged] #184

Closed icex2 closed 1 year ago

In GitLab by @xyen on Mar 10, 2021, 08:55

Merges aciomgr -> master

Summary

Adds ACIO manager to BT

Description

ACIO Manager, or aciomgr, is used to allow multiple separate DLLs in the same process access to the same ACIO bus. ex: KFCA and ICCA.

This PR also updates the relevant io DLL's, and fixes some minor issues with eamiotest.

Related Issue

62

How Has This Been Tested?

Tested by making sure both eamiotest and aciotest with eamio-icca work with an ICCA I have, as well as sdvxio-kfca on a cab.

In GitLab by @xyen on Mar 10, 2021, 09:19

added 1 commit

d3e98e41 - eamio-icca: fix config prefix

Compare with previous version

Do we actually have to introduce deprecation? This is not part of any "public" API and all (known) dependencies are part of BT5.

Are there any disadvantages if we make all code use acio mgr even if they just use a single device?

Nit: Avoid magic number on char product[4] -> char product[ACIO_NODE_PRODUCT_CODE_LEN]

What does "checkout" mean here?

Now seeing checking, is it binding it to the current thread? Not sure what's a good terminology here, alternatives: "acquire"/"release" (though this is rather used for locks/semaphores), "bind"/"unbind"?

That's a very non transparent DllMain that I would not expect there. I assume you want to achieve having aciomgr initialized/shutdown. Would it be possible to add this explicitly to the applications/DLLs that actually need it? This would break the way, we setup any kind of hooking/emulation code in the various hook libraries.

Nit: Code style, empty line after control block.

// warn ?

Same here. Did you want to to do some logging here?

I see a few more of these ahead, not repeating my comments to avoid more noise.

dispatcher->references is not atomic, so depending on the timing, we might run into visibility issues here if not part of the critical section. Suggestion: Make that size_t references var an atomic_int?

:thumbsup: for the explanation here.

Nit: What you are stating in the comment is quote obvious imo.

Same here.

Nit: Magic number 4 (also further down). Replace with defined macro (see previous comment).

Looking at the code, the function name might be somewhat misleading. Maybe a brief comment that the base functionality is identical on these models might be helpful.

Some more magic number nit: I would move these to some macros at the top of the file:

#define COM_PORT_ICCA_DEFAULT "COM1"
#define BAUD_RATE_ICCA_DEFAULT 57600

Nit code style: Empty line before and after control blocks (also in the code following this line).

Apply the clang code style on all source files: make code-format

Nit: Another magic number with -1 -> #define INVALID_NODE_ID -1 improves readability imo (also further down).

In GitLab by @xyen on Mar 14, 2021, 14:34

Commented on src/main/aciomgr/manager.c line 205

No, because the locks synchronizing everything must be setup before anything else can be called. Doing it here doesn't break how our hooks or emulation code works at all.

Ok, then this would be just about increased visibilty of that: I suggest to move the DllMain function to a separate dllmain.c module if possible.

In GitLab by @xyen on Mar 15, 2021, 24:53

Commented on src/main/aciomgr/manager.c line 205

sounds good to me, will do

In GitLab by @xyen on Mar 16, 2021, 01:32

Commented on src/main/aciodrv/device.h line 23

additional complexity, also I added this so that when new usages of this are added, it forces the dev to think about th e use-case, and be clear with which one they use.

In GitLab by @xyen on Mar 16, 2021, 01:36

Commented on src/main/aciomgr/manager.h line 71

checkout the device handler from the manager

In GitLab by @xyen on Mar 16, 2021, 01:37

Commented on src/main/aciomgr/manager.h line 80

checkout/checkin sound fine to me, since yeah acquire/release is used for locks, bind/unbind sound more permanent.

In GitLab by @xyen on Mar 16, 2021, 02:39

Commented on src/main/eamio-icca/eamio-icca.c line 59

internally they're all referred to as ICCA, but yeah, will add a comment.

In GitLab by @xyen on Mar 16, 2021, 02:52

Commented on src/main/eamio-icca/eamio-icca.c line 112

calling that causes changes in over 100 files, will make a PR later that does this

In GitLab by @xyen on Mar 16, 2021, 03:42

resolved all threads