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.