search

Rahix / avr-device

Register access crate for AVR microcontrollers
Apache License 2.0
170 stars 66 forks source link

Add attiny841 and attiny861 #67

Closed jaxter184 closed 3 years ago

jaxter184 commented 3 years ago

Preliminary pull request adding attiny841 and attiny861 chips to go with Rahix/avr-hal#105.

jaxter184 commented 3 years ago

I still need to figure out the timer stuff, and I probably want to look through the patches a little more closely to make sure that I'm doing it correctly

jaxter184 commented 3 years ago

I think I've just about figured out the timer stuff, with one caveat: In patch/timer/dev, there are a 2 different 16-bit timer patches. The primary difference that I could distinguish is that one describes that OCix is set/cleared at TOP rather than BOTTOM. However, the datasheet register descriptions are the same between the devices that use 16bit-tiny84-tc1.yaml (attiny84 table 14-3) and the devices that use 16bit.yaml (for example: atmega2560 table 20-3), namely that both say to set and clear at BOTTOM. Am I reading these wrong? Is there an error? Should I be looking at a different part of the datasheet for this information?

Rahix commented 3 years ago

I think I've just about figured out the timer stuff, with one caveat: In patch/timer/dev, there are a 2 different 16-bit timer patches. The primary difference that I could distinguish is that one describes that OCix is set/cleared at TOP rather than BOTTOM. However, the datasheet register descriptions are the same between the devices that use 16bit-tiny84-tc1.yaml (attiny84 table 14-3) and the devices that use 16bit.yaml (for example: atmega2560 table 20-3), namely that both say to set and clear at BOTTOM. Am I reading these wrong? Is there an error? Should I be looking at a different part of the datasheet for this information?

Looks like this slipped through and you're right, they are set/cleared at BOTTOM. The other difference between those files is the TIFRn register. Maybe that doesn't exist on the ATtiny version?

jaxter184 commented 3 years ago

I think I've just about figured out the timer stuff, with one caveat: In patch/timer/dev, there are a 2 different 16-bit timer patches. The primary difference that I could distinguish is that one describes that OCix is set/cleared at TOP rather than BOTTOM. However, the datasheet register descriptions are the same between the devices that use 16bit-tiny84-tc1.yaml (attiny84 table 14-3) and the devices that use 16bit.yaml (for example: atmega2560 table 20-3), namely that both say to set and clear at BOTTOM. Am I reading these wrong? Is there an error? Should I be looking at a different part of the datasheet for this information?

Looks like this slipped through and you're right, they are set/cleared at BOTTOM. The other difference between those files is the TIFRn register. Maybe that doesn't exist on the ATtiny version?

It looks like TIFRn does exist in the ATtiny84. Should I maybe consolidate it into one file? Is it bad practice to put multiple unrelated changes in a single pull request?

Rahix commented 3 years ago

It looks like TIFRn does exist in the ATtiny84. Should I maybe consolidate it into one file?

If possible, that would of course be the best!

Is it bad practice to put multiple unrelated changes in a single pull request?

I guess this is highly project specific. Here's my take: I think it is very important that every "atomic" change goes into a separate commit. I.e. every change that makes "sense" on its own and can stand without the following changes being needed should be separated.

The reason for this is that this means the git history is a clean record of what was done with the project. If I find a bug in a few years, I can just git blame and find out what was changed and ideally also why (if a proper commit message was written). Similarly, if something stops working at some point I can find the exact change that introduced a regression using git bisect.

If multiple changes happen in a single commit or if an "atomic" change is split out over multiple commits, this is of course not going to work effectively.

Now, to archieve this clean separation, there are two ways:

I see that you've already followed the first route which is really nice (thanks a lot!). So I have no problem if you add more changes into this series. A minor nag if you want to make it even more perfect: You could reorder the commits slightly so that each one could possibly compile (right now the ATmega328PB fix is after the changes to the AC common patch). But that's really a minor detail.

On a side note: There is currently a merge conflict. Please rebase on master to fix :)

jaxter184 commented 3 years ago

I've rebased the commits, and assuming my testing procedure was correct, they are all makeable.

Rahix commented 3 years ago

Thanks for your work in this PR!