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.

Sign up for GitHub

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

Jump to bottom

Complete sn32f2xx support, add functionality #5

Merged
merged 46 commits into from
Sep 14, 2024
Merged

Complete sn32f2xx support, add functionality #5

merged 46 commits into from
Sep 14, 2024

Conversation

dexter93
Copy link
Contributor

@dexter93 dexter93 commented Sep 6, 2024 •
edited
Loading

Supports all sn32f2xx chips
Add options for flash erase, CS re/set and data verification
Simplify and correct the protocol usage

Automagic chip identification, no longer VID/PID bound

@dexter93
add 290 support
40f1e00
@dexter93
add 240C support
dfd004f
@dexter93
abstract buffer size
9206e7e
@dexter93
abstract hid reports size from actual payload
8c3fd32
@dexter93
rename commands to properly reflect ISP protocol
523b35e
@dexter93
v2.0.1
7dbb528 
abstract functions from the flash function
further compliment the api, introduce support for new chips
actually validate the flash status
new options
fix various bugs
@dexter93
debug: print out verbose fails
4276fed
@dexter93
debug: try to print error states
e841dc9 
this is platform dependent - not implemented in all platforms
@dexter93
correct cs location
b444a43
@dexter93
add better password checks
fe329aa 
and bail if any part fails
@dexter93
print out supported vid pid for known chips
cf2fc27
@dexter93
fail if chip version is not supported
0eefacf
@dexter93
fix bad --offset argument reqs
916e9c5
@dexter93
280: preliminary support
8f79b95
@dexter93
erase: correct behavior
58a43dd 
we should parse pages, not kb size
@dexter93
add 280 series PID
f350501
@dexter93
update buffer handling to send 64b always
288e1a6
@dexter93
rename isp password check
5608960 
we're actually doing code option table checks here
@dexter93
slight cleanup
a23b8bd
@dexter93
v2.0.2 bump
f9670dd
@dexter93 dexter93 requested review from Jpe230 and stdvar September 6, 2024 20:37
@Jpe230
Copy link
Collaborator

Jpe230 commented Sep 7, 2024

Woops, vscode had a fart and each suggestion was sent individually, sorry.... Still, lgtm, just some formatting suggestion....

karliss
karliss reviewed Sep 9, 2024
View reviewed changes
Copy link

@karliss karliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick for the description of PR - this doesn't cover all the SN32F200 chips. It doesn't take into account SN32f239 and SN32f229. Not suggesting it's necessary to support them, just that the description could be adjusted.

sonixflasher.c Outdated Show resolved Hide resolved
sonixflasher.c Outdated Show resolved Hide resolved
sonixflasher.c Outdated Show resolved Hide resolved
sonixflasher.c Outdated Show resolved Hide resolved
sonixflasher.c
write_buffer_16(buf + 6, CS0); // WARNING THIS SETS CS0
if (!hid_set_feature(dev, buf, REPORT_SIZE)) return false;
if (!hid_get_feature(dev, buf, CMD_SET_ENCRYPTION_ALGO)) return false;
clear_buffer(buf, REPORT_SIZE);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless clearing of local stack buffer which is about to go out of scope. Same thing in next few functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly being extra careful with buffer management here while implementing undocumented bits of the protocol

@dexter93
Copy link
Contributor Author

Minor nitpick for the description of PR - this doesn't cover all the SN32F200 chips. It doesn't take into account SN32f239 and SN32f229. Not suggesting it's necessary to support them, just that the description could be adjusted.

Mostly avoided them because of bad/missing docs. However, looking at it again the protocol is compatible and they're simply a revision of 240

index,bl_version,family,revMajor,revMinor
SN32F249
12,0x01 0x20 0x00 0x03
SN32F239
13,0x01 0x20 0x00 0x02
SN32F229
14,0x01 0x20 0x00 0x01

That also means the version check atm is buggy, since it will identify 230 and 220 chips as 240 ( issues arising from this are related to flash size on erase/program). Definitely a nice find, will amend

karliss
karliss approved these changes Sep 10, 2024
View reviewed changes
Copy link

@karliss karliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good enough for me.

With which chips has the new code been actually tested?

@dexter93
Copy link
Contributor Author

Testing being done on specific platform samples
240b works ( covers also 260)
290 works ( covers also 280, 240C)
240 tested up until v2.0.3 - worked fine ( should also cover the now unobtainable 220/230)

HorrorTroll
HorrorTroll approved these changes Sep 13, 2024
View reviewed changes
Copy link

@HorrorTroll HorrorTroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

240 series work good on latest commit.

@dexter93 dexter93 merged commit 2a46977 into SonixQMK:main Sep 14, 2024
4 checks passed
@dexter93 dexter93 mentioned this pull request Sep 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HorrorTroll HorrorTroll HorrorTroll approved these changes

@karliss karliss karliss approved these changes

@Jpe230 Jpe230 Awaiting requested review from Jpe230

@stdvar stdvar Awaiting requested review from stdvar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dexter93 @Jpe230 @karliss @HorrorTroll