Make full use of all hardware acceptance masks and filters

[xyx0826]

Addresses #19.

This PR implements logics to allow for storing multiple message filters under the same mask. Changes validated using a custom RP2040/MCP2515 board on a CAN bus using three different message filters (matches).

[jepler]

thanks, this looks like a good improvement!

There are some changes needed for the automated checks, and I also have some feedback.

Please feel free to @ me when you have updated the branch, or if you have questions about my request or the automated checks.

[xyx0826]

Thanks for looking over this! I will get to fixing the check issues later this week. Also I might've missed something but I can't seem to find any of your comments or requested changes on the change list.

[tekktrik]

Heads up, pre-commit has been updated, so automated checks may yield something different! I think I retriggered the run while patching things.

[HourGlss]

I would love it if this got approved and Masks and filters were updated in the docs as well. I currently have an issue open that relates to Masks as well, hopefully this fixes that too.

[jepler]

This data should be an instance member (self.masks_filters), because otherwise a system with more than one MCP2515 chip in it would not function properly.

[jepler]

I'm sorry, my old comment(s) were apparently never submitted. I looked back at the PR today and besides the automated checks that are failing, I think that the use of a global variable should be replaced by an instance variable.

[xyx0826]

Making acceptance data instanced makes more sense. Thanks for the review, I'll get to implementing those changes.

[HourGlss]

I can help with testing if you want. I really want to make masks as useful as possible. Please remove the print for 1 mask only.

.fyx on discord

[xyx0826]

I can help with testing if you want. I really want to make masks as useful as possible. Please remove the print for 1 mask only.

.fyx on discord

Testing is appreciated as I no longer have a MCP2515 setup by my hand. What do you mean by the print? This PR doesn't actively print anything.

[HourGlss]

check out this issue: #22

This PR is against this issue: #19 I get it. But if we simply include testing with 1 mask we will hit the above issue and can fix it.

If we are going to fix masks, let's fix masks. Adding documentation for Mask would be a good step also.

[anecdata]

There is documentation for the canio module that may be useful
https://docs.circuitpython.org/en/latest/shared-bindings/canio/index.html#canio.Match

[HourGlss]

keeping consistency with canio is a must IMO. diverging from their use of Match is not a good idea.

[xyx0826]

Changes implemented. @jepler Can you help me track down what's being reformatted on line 1? I can't get pylint to lint the same way locally.

[HourGlss]

Did you need me to test? You never reached out via discord.

[jepler]

@tekktrik hey do you know why the change (diff) introduced by black for this PR is not being shown in the results?

@xyx0826 These are the changes made locally when I run pre-commit run --all-files and commit the result:

commit e44aa94985e1df58d2b17650f783e47d32362a80
Author: Jeff Epler <jepler@gmail.com>
Date:   Sat Jan 20 15:42:11 2024 -0600

    re-format per black pre-commit

diff --git a/adafruit_mcp2515/__init__.py b/adafruit_mcp2515/__init__.py
index 9d7e6f4..f27e6d1 100644
--- a/adafruit_mcp2515/__init__.py
+++ b/adafruit_mcp2515/__init__.py
@@ -317,16 +317,11 @@ class MCP2515:  # pylint:disable=too-many-instance-attributes
         self._loopback = loopback
         self._silent = silent
         self._masks_filters = {
-            _RXM0SIDH: [None, {
-                _RXF0SIDH: None,
-                _RXF1SIDH: None
-            }],
-            _RXM1SIDH: [None, {
-                _RXF2SIDH: None,
-                _RXF3SIDH: None,
-                _RXF4SIDH: None,
-                _RXF5SIDH: None
-            }]
+            _RXM0SIDH: [None, {_RXF0SIDH: None, _RXF1SIDH: None}],
+            _RXM1SIDH: [
+                None,
+                {_RXF2SIDH: None, _RXF3SIDH: None, _RXF4SIDH: None, _RXF5SIDH: None},
+            ],
         }
 
         self._init_buffers()
@@ -819,8 +814,9 @@ class MCP2515:  # pylint:disable=too-many-instance-attributes
     def _create_mask_and_filter(self, match: Match):
         actual_mask = match.mask
         if match.mask == 0:
-            actual_mask = EXTID_BOTTOM_29_MASK if match.extended \
-                else STDID_BOTTOM_11_MASK
+            actual_mask = (
+                EXTID_BOTTOM_29_MASK if match.extended else STDID_BOTTOM_11_MASK
+            )
 
         result = self._find_mask_and_filter(actual_mask)
         if not result:
@@ -828,7 +824,7 @@ class MCP2515:  # pylint:disable=too-many-instance-attributes
                 "No mask and filter is available for "
                 f"mask 0x{actual_mask:03x}, addr 0x{match.address:03x}"
             )
-        
+
         (mask, filt) = result
         self._set_acceptance_register(mask, actual_mask, match.extended)
         self._set_acceptance_register(filt, match.address, match.extended)
@@ -839,12 +835,11 @@ class MCP2515:  # pylint:disable=too-many-instance-attributes
         """Clears the Receive Mask and Filter Registers"""
         for mask_reg, mask_data in self._masks_filters.items():
             self._set_register(mask_reg, 0)
-            mask_data[0] = None # Mask value
+            mask_data[0] = None  # Mask value
             for filt_reg in mask_data[1]:
                 self._set_register(filt_reg, 0)
                 mask_data[1][filt_reg] = None
 
-
     ######## CANIO API METHODS #############
     @property
     def baudrate(self):
@@ -944,7 +939,6 @@ class MCP2515:  # pylint:disable=too-many-instance-attributes
                 # Mask unused, set it to a match to prevent leakage
                 self._set_acceptance_register(mask_reg, match.mask, match.extended)
 
-
         return Listener(self, timeout)
 
     def deinit(self):

[tekktrik]

@jepler if I understand your question correctly, I don't think black typically announces via diff what it changed, just that it did change a certain number of files, and the result error code causes the CI to fail. That's at least my experience.

[tekktrik]

I realize now that you mean the failure help text, which has an error. That is a good question, I can look into that!

[jepler]

I opened a PR over in adafruit/workflows-circuitpython-libs#34 for showing the changes introduced by black from pre-commit.

[FoamyGuy]

I tried to merge main to this branch and resolve conflicts but it appears that maintainers do not have permission to update the PR branch.

[xyx0826]

I'm no longer involved with UW FSAE (the organization I was part of that used this project) and won't have time to improve on this PR any further. Please feel free to take this work and start your own PR though.