RevolutionPi / piControl

Kernel module for data exchange with RevPi I/O-Modules and Gateways
81 stars 24 forks source link

Add support Connect 4 (classic piControl) #94

Closed nbuchwitz closed 1 year ago

nbuchwitz commented 1 year ago

This is my first series for RevPi Connect 4 support and targets the master branch. Commits are nearly 100% identical to the ones in #91

l1k commented 1 year ago

In the commit message of ef1f4f4784a5f74b936ea56b526ebf9c78ab974d, s/A2 blue/A1 blue/.

l1k commented 1 year ago

Commit e0ed45caf7bf is missing a number of conversions to the new flags:

If this was something that's going to be upstreamed into the mainline kernel, you'd want to include all conversions possible so that the diffstat shows the commit is worthwhile accepting (because it reduces LoC).

l1k commented 1 year ago

I note that there are a couple of places where the code distinguishes between Core/CoreSE and Connect/ConnectSE in order to perform certain operations that are only necessary if there's a PiBridge on the right handside. It might be worthwhile having a flag for that as well.

nbuchwitz commented 1 year ago

Commit e0ed45c is missing a number of conversions to the new flags:

* PiBridgeMaster.c: `PiBridgeMaster_Stop()`, `PiBridgeMaster_Continue()`, `PiBridgeMaster_Run()` (four missing conversions for `pibridge_ethernet_p2p_supported`)

* piControlMain.c: `waitRunning()` (return true `if (!piDev_g.pibridge_rs485_supported)`)

If this was something that's going to be upstreamed into the mainline kernel, you'd want to include all conversions possible so that the diffstat shows the commit is worthwhile accepting (because it reduces LoC).

Not only there, found a few more. Thanks for the hint!

nbuchwitz commented 1 year ago

I note that there are a couple of places where the code distinguishes between Core/CoreSE and Connect/ConnectSE in order to perform certain operations that are only necessary if there's a PiBridge on the right handside. It might be worthwhile having a flag for that as well.

Make sense. I've added another flag called only_left_pibridge. The logic is chosen deliberately as this is an exception from the "usual" PiBridge functionality.

I really like having these flags as it makes the integration of a new device (besides LEDs and other) quite easy.

l1k commented 1 year ago
I really like having these flags as it makes the integration of a new device (besides LEDs and other) quite easy.

Right and in addition something like

    if (piDev_g.revpi_gate_supported)
        revpi_gate_init();

is the kind of idiomatic C that's common in the kernel.