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

Serial attach interrupt #95

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

Conversation

gicking
Copy link

@gicking gicking commented Nov 23, 2019

Added option to attach custom functions to Serial receive and transmit interrupts.
Receive is similar to NeoHWSerial by SlashDevin but

  • works transparently on AVR and SAM
  • user function is called after reception of each byte
  • in addition to received byte, also UART status is passed. This allows e.g. frame synchronization via framing errors as in LIN

Transmit is new:

  • works transparently on AVR and SAM
  • user function is called after all bytes have been send and transmission is complete
  • this allows e.g. custom protocol handlers in interrupts (like LIN)

Attached please find an example (tested for Mega and Due). Connect Tx1 & Rx1 and check serial monitor. A separate pull request was started for AVR (see here)
Serial_attach_interrupt.zip

added optional custom handler to Serial interrupt. Is similar to
NeoHWSerial by SlashDevin (https://github.com/SlashDevin/NeoHWSerial),
but:

- user function also gets UART status as parameter to e.g. detect LIN
break in user routine.

- ported to Arduino Due

Added as core extension because AFAIK cannot be implemented as library
for Due. Also note similar pull request for AVR
(arduino/ArduinoCore-avr#297)
- add Serialx.attachInterrupt_Send()
- rename Serialx.attachInterrupt() to Serialx.attachInterrupt_Receive()
- updated modification date to sync with SAM core
- no functional change
@gicking
Copy link
Author

gicking commented Nov 24, 2019

I believe I just found a bug which causes a stall to the attachSend() -> please wait before merge! I will update when/if I fixed it. Sorry!

Done, see below

- activate custom send_ISR also for 1B write
- previous solution only worked for >=2B writes
@gicking
Copy link
Author

gicking commented Nov 24, 2019

ok, now the bug is fixed in my branch, also see above commit 1557f9c. Specifically for 1B write, the UART TXEMPTY interrupt is now activated specifically, not in the TXRDY interrupt after the last byte is written.

Question: is the pull request automatically updated to include the fix automatic

@per1234
Copy link
Contributor

per1234 commented Nov 25, 2019

Question: is the pull request automatically updated to include the fix automatic

@gicking yes. Anything you push to the branch the pull request is based on automatically becomes part of the pull request.

@gicking
Copy link
Author

gicking commented Nov 29, 2019

Question: is the pull request automatically updated to include the fix automatic

@gicking yes. Anything you push to the branch the pull request is based on automatically becomes part of the pull request.

hi per1234,

thanks a lot for your confirmation! Have a great day :-)

@gicking
Copy link
Author

gicking commented Nov 29, 2019

Note: a discussion is ongoing in the respective AVR "pull request page" about whether Serial.attachInterrupt_*() can and should be done as a library instead of a core extension. So please merge yet. Thanks!

@gicking
Copy link
Author

gicking commented Jan 21, 2020

Note: in the parallel discussion for AVR a proposal was made by matthijskooijman to separate Serial similarly to AVRs. In that case no core extension for AVR or SAM would be required, a library like NeoHWSerial would do. This option is preferrable to be, so please don't merge this pull-request just now, but wait for the SAM discussion

@matthijskooijman
Copy link
Collaborator

To keep the discussion clean, I created new issues for the "separate serial objects" implementation here: #100 and arduino/ArduinoCore-samd#489. Let's keep this issue about adding the serial interrupt API.

@gicking
Copy link
Author

gicking commented Jan 25, 2020

@matthijskooijman: if the serials for SAM can be implemented similar to AVR, Rx- and Tx-interrupts can be added via a library like NeoHWSerial. In that case I would withdraw this pull request.

For now I propose to leave it open and discuss it in the above issues arduino/ArduinoCore-sam#100 and arduino/ArduinoCore-samd#489, you created.

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

@gicking
Copy link
Author

gicking commented Apr 10, 2021

seriously? It takes 16 months to automatically send a legal form...???

@vChavezB
Copy link

vChavezB commented Dec 22, 2021

Hi @gicking ,
this is a nice feature to have direct access to the RX interrupts of the UART/USART peripherals in the ATSAM3x8e.

Have you had any update on whether this will be merged?

I think that the Arduino SDK should give the user access to the interrupts either via class methods like yours or any other type of hook implementation. I mention this because with a project I am working we need all the 4 USART ports. As the data has to be processed directly via interrupts, the Serial class is limited as you only have access to the RX buffer from the polling methods: Serial::Available,Serial::read. This means if you need to process RX interrupt events in real-time its not possible.

At the end what we did is deleted the USARTx_Handlers in the variants.cpp to suit our needs and override the weak USARTx_Handlers with our implementation.

With your feature I could call the Arduino SDK function and would not need to modify the variants.cpp file.

Also may I suggest to add a void * argument parameter to isrRx_t. If I am using a callback method (static) that resides within a class I could pass as argument the instance of the object (this), typecast it and in my callback I can use the data from the object. Otherwise I am limited with the functionality of callbacks.

Example

class MyClass
{
   static void MyMethod(uint8_t data,uint32 status,void * args)
   {
      MyClass * pMyClass = static_cast<MyClass *>(args);
      //Do something with the data of the instance
   }
};

MyClass classInstance;
void setup()
{
    attachInterrupt_Receive(MyClass::MyMethod,static_cast<void *>&classInstance);
}

@gicking
Copy link
Author

gicking commented Dec 23, 2021

hey @vChavezB ,

unfortunately there was no response after April 2021. A similar issues was raised by @matthijskooijman, see here and here, also with zero response by the core team. And similar pull requests from 2017 by @facchinm and 2019 by @anklimov are also still open. In general I get the feeling that support for the SAM boards was dropped altogether.

As for your request for an ISR with parameters, I'm not sure I understand. The ISR is not called by the SW but by the interrupt controller. So who would provide the argument during runtime? Or do you propose to add a callback pointer only for attachInterrupt_Receive(), which is stored in a static variable and called by the ISR...? If yes, you can just as well do this in your custom routine - I am confused...

For your explanation thanks a lot in advance!

Georg

@vChavezB
Copy link

unfortunately there was no response after April 2021

yes, from what I see this board development is not active. As the project we are working on will be released and I have modified directly this repo I think I will make my own board based on this repo with the json board definition so its accessible in the IDE as I do not see any more features being integrated here...

About the callback with an extra void argument...

The advantage I see of this argument (void *) is that it gives context to the callback. Yes you could do this inside the custom routine but it limits what you can do.

For example, with the extra argument you can have access to the members of a class

class MyClass
{
  public:
       static void MyMethod(uint8_t data,uint32 status,void * args)
       {
            MyClass * pMyClass = static_cast<MyClass *>(args);
            pMyClass->count++;
       }
       int count=0;
};

Without this argument, it would be more difficult to interact with an object instance in the callback. Which given the nature of the Arduino SDK (C++) I think its a good idea, since it allows an OOP approach.

So How could you increment the count member of class MyClass without this extra argument?

  1. Create a global instance of the class and use it in the callback
MyClass myInstance;
void MyClass::MyMethod(uint8_t data,uint32 status)
{
            myInstance.count++;
}

What happens if you have more instances? Then you woul need to hardcode the instance you want to use for the callback

MyClass myInstance[2];
void MyClass::MyMethod1(uint8_t data,uint32 status)
{
            myInstance[0].count++;
}
void MyClass::MyMethod2(uint8_t data,uint32 status)
{
            myInstance[1].count++;
}
  1. Use templates as mentioned in stackoverflow

As you see, passing an extra argument allows to use the callback within a class in a much simpler way. So the extra argument gives the context to the callback. Also when you think about programming a Driver for Hardware in C, you normally have an API which gives you opportunity for a void * arg . As a quick example, I found this struct in the linux kernel which uses a void* param for a callback

@gicking
Copy link
Author

gicking commented Dec 23, 2021

hi again,

do you propose the ISR calls the attached function with the received byte, the UART status, and the optional void* argument - which may be a callback function...? Sorry, I still don't quite get it...

Therefore I propose you create a pull request to my repo, and provide an example how the argument is passed. Thanks in advance!

Georg

@vChavezB
Copy link

Hi Georg,

I already started tinkering around with this repo. I added the callback with argument parameter here:

https://github.com/vChavezB/ArduinoCore-sam

I also added support for the USART2 peripheral of the Atsam3x8e.

As I do not know if the mantainers are interested in adding these features I think I will continue with a fork and a custom boards package since the compiler version is really old.

Regards
Victor

added optional argument for user ISR function, see proposal from  arduino#95 (comment)
@gicking
Copy link
Author

gicking commented Dec 24, 2021

hi again,

thanks for the clarification, now I understand :-) I have added the void* argument to my repo and this pull request - for whatever it's worth...

Have a nice X-mas and a Happy New Year!

Regards, Georg

gicking added a commit to gicking/ArduinoCore-avr that referenced this pull request Dec 24, 2021
@gicking
Copy link
Author

gicking commented Dec 24, 2021

seems like our impression is correct, see this discussion on Due development status

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.

5 participants