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

atmega32: Add ATmega32 and ATmega324p support. Implement simpler-api branch #138

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jamesrwaugh
Copy link

@jamesrwaugh jamesrwaugh commented Feb 17, 2023

Hello,

This merge adds a first pass at ATmega32 and ATmega324p support.

I did this to expand avr8js's scope a little bit to be a more general-purpose AVR simulator (as other AVR options are old and not as easy to use, mostly in C), as my projects use these chips, and to lay any groundwork for refactoring into allowing adding any new chips to be easier.

Key part of this merge

  • The default chip is still left as the 328p.
  • Other chips only had peripheral changes: the 32 needing a slightly different USART implementation, and the 324p being exactly the same as the 328p except for interrupts addresses.
  • The Mega (2560) was split out into its own files; previously it was more or less intertwined with the 328p.
  • The project exports have been left as is. I don't think I have a say on if this can be a breaking change or not.
  • Similarly, any simpler-api changes are not exported, I will leave that up to the maintainers (Simpler API? Request for comments #87).

I have not:

  • Tested any of these on real hardware to confirm any feature parity.
  • Examined to see if there are any instruction set or core CPU differences except a quick skim.

@urish urish self-assigned this Feb 19, 2023
@urish urish added the enhancement New feature or request label Feb 19, 2023
@urish
Copy link
Contributor

urish commented Feb 19, 2023

Thank you very much!

A few questions:

The project exports have been left as is. I don't think I have a say on if this can be a breaking change or not.

  1. If we didn't care about making breaking changes, what would you do differently?
  2. What are your thoughts about moving all the chip specific configuration to live in the chip definition file itself, instead of having it scattered across multiple files (spi_atmega324p.ts, twi_atmega324p.ts)?
  3. How did you test the atmega324 / atmega32 implementation? What test programs did you run, how did you compile them?

@jamesrwaugh
Copy link
Author

If we didn't care about making breaking changes, what would you do differently?

I would prefer just createAvr with some more options, such as initial program bytes and a clock speed. The peripherals should be exported behind some internal namespace in case the types are needed, but should not normally used. But I think createAvr needs some improvements first.

  1. Give an explicit name type to the return value of createAVR to allow explicitly referring to its type.
  2. Add in more options such as program for initial program bytes at least.

I envision something like this:

  const program = loadHex(MyIntelHexString);

  const AVR = createAVR(ATmega324p, { program: program, clockSpeedHz: MHZ });

  AVR.gpio['B'].addListener((value, oldValue) => {
    console.log(value);
  });

  AVR.usart[0].onLineTransmit = (value) => {
    console.log(value);
  };

What are your thoughts about moving all the chip specific configuration to live in the chip definition file itself

That's perfectly fine, I was mostly following status-quo to avoid a massive un-reviewable pull request.

How did you test the atmega324 / atmega32 implementation?

Not very extensively yet. Only GPIO, timers, and timer interrupts.

I have taken parts of the Arduino core (digital/AnalogRead/Write, Print.cpp, ...) along with the timer vectors and delay() implementation out of Arduino into a standalone library on my end, and compiled with standard C, with no Ardunio software.

I compiled them using standard avr-gcc and pasted the hex firmware into the JavaScript.

@urish
Copy link
Contributor

urish commented Feb 20, 2023

Thanks James!

I like your suggestion. Refactoring createAVR() would not even be a breaking change, since we never released it. And in any case, I'm open to breaking changes - the API of this library was designed 3 and half years ago. It's okay to change if the changes make sense.

Is refactoring createAVR() the way you suggested is something that you have time to do? Or are you busy with other things?

The pull request is already massive, but from what I've seen so far you adapted the code style of the project pretty quickly, and the code quality is great. So I wouldn't worry about adding stuff into the PR.

Where can I find the standalone library that you created on your end? Ideally, I'd like to be able to reproduce your setup and eventually turn it into a Github action that will run on every push and ensure we have no regressions.

@jamesrwaugh
Copy link
Author

I'm still reviewing the test code, let me get back to you
I pushed it here, but it's not ready for prime time yet.
https://github.com/jamesrwaugh/arduino-corelib

@jamesrwaugh
Copy link
Author

@urish I've taken a comb over the above project and tested everything (gpio output/input/input pullup, analog read, serial read/write) on an actual ATmega324p. So I am confident that the simulator should reflect accurate behavior.

Unfortunately my time lately has been focused away from this project. I added a bit more to createAvr but I would leave this up to you for now if you wanted to add any touches to merge it in.

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

Successfully merging this pull request may close these issues.

None yet

2 participants