Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added additional mcu for a CTRL with a different chip id. #25

Closed
wants to merge 1 commit into from

Conversation

pgillan145
Copy link

My new CTRL High-Profile had a different chip ID than what was listed in the mcu structure, so I added an addition entry. Tested using mdloader to write a new keymap against both my new CTRL and the original low-profile without issue.

@trent-boyd
Copy link

Thanks for the fix, @pgillan145! I compiled this on my Mac and it updated my new High-Profile just fine. :)

cc: @just-another-jxliu

@fauxpark
Copy link

#24 (comment) is a cleaner solution, that doesn't require an entirely new entry be created.

@pgillan145
Copy link
Author

#24 (comment) is a cleaner solution, that doesn't require an entirely new entry be created.

How's it cleaner? It seems like a hack that relies on coincidence, and if a device came along with a chip id that happened to fit the pattern it would fail. The whole point of the mcu array is to support multiple chips, and that's what this seems to be, a new chip.

@fauxpark
Copy link

fauxpark commented Jul 10, 2020

It's not a new chip, it's the same chip (ATSAMD51J18A) with a different revision ID. See Table 1 from this document: http://ww1.microchip.com/downloads/en/DeviceDoc/SAM-D5x-E5x-Family-Silicon-Errata-DS80000748K.pdf
Since only the revision ID is subject to change, it has to be masked off, which is what my suggestion does.

@pgillan145
Copy link
Author

(NOTE: I know this in an incredibly minor and inconsequential change, I'm just continuing the discussion because I like to argue and I don't have much going on today.)

You can't claim something's the same if the primary id changes -- if the unique identifier changes, it's different, by definition. The correct solution in this case would be to modify the mcu list so it included the mask, or some other way to indicate the "revision" digit. Also, your change is totally opaque, and anyone looking at it would have no idea why it's there. At the very least it requires a comment.

I also had a couple of other points about another random chip having the same pattern, or the revision number exceeding 16, but the data sheet makes those pretty much irrelevant.

@fauxpark
Copy link

fauxpark commented Jul 11, 2020

You can't claim something's the same if the primary id changes -- if the unique identifier changes, it's different, by definition.

Tell that to Microchip, I guess? They're the ones saying it's the same part number. The revision number changing doesn't make it a different part, thus the xs. Code written for the rev A SAMD51s should be reasonably expected to work just as well or better on a rev D (mostly because the compiler doesn't know or care).

Also, your change is totally opaque, and anyone looking at it would have no idea why it's there. At the very least it requires a comment.

I didn't file a PR or anything, I just said "change this line to this". You're free to add whatever comments you want. But yes, I probably should have clarified what it does exactly.

@just-another-jxliu
Copy link

Thanks, all. We applied the change suggested by @fauxpark : #24 (comment)

Newly-compiled files are here: https://github.com/Massdrop/mdloader/releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants