Open xyx0826 opened 1 year ago
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.
Heads up, pre-commit
has been updated, so automated checks may yield something different! I think I retriggered the run while patching things.
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.
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.
Making acceptance data instanced makes more sense. Thanks for the review, I'll get to implementing those changes.
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
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.
check out this issue: https://github.com/adafruit/Adafruit_CircuitPython_MCP2515/issues/22
This PR is against this issue: https://github.com/adafruit/Adafruit_CircuitPython_MCP2515/issues/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.
There is documentation for the canio
module that may be useful
https://docs.circuitpython.org/en/latest/shared-bindings/canio/index.html#canio.Match
keeping consistency with canio is a must IMO. diverging from their use of Match is not a good idea.
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.
Did you need me to test? You never reached out via discord.
@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):
@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.
I realize now that you mean the failure help text, which has an error. That is a good question, I can look into that!
I opened a PR over in https://github.com/adafruit/workflows-circuitpython-libs/pull/34 for showing the changes introduced by black from pre-commit.
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).