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

Remove Arduino dependencies #33

Closed
denravonska opened this issue Nov 22, 2016 · 64 comments · Fixed by #35
Closed

Remove Arduino dependencies #33

denravonska opened this issue Nov 22, 2016 · 64 comments · Fixed by #35

Comments

@denravonska
Copy link
Contributor

denravonska commented Nov 22, 2016

I know this library was written with Arduino in mind, but I think that with some smaller modifications it could be made platform agnostic. Have a look at my suggestions and let me know what you think. I can fix these in my fork and submit a pull request.

  • Remove Arduino.h includes.
  • Replace bit operations with shifts. B00000100 becomes (1 << 2).
  • Move debug print operations to a separate N2kDebug.h/cpp.
  • Replace bracket includes with quotation includes for local files (merely cosmetic, but I got a compiler warning. #include <N2kMessages.h> becomes #include "N2kMessages.h".
  • Replace lowByte/highByte with shift operations. highByte(v) becomes (v >> 8) & 0xFF.
  • Leave millis() as it is and declare it external where it's used. This and CAN would be the application requirements. On Arduino it should work as-is while other platforms can wrap their own clock inside their millis function.
  • Functions which just want a Stream to read from or write to could accept functors instead.

The examples would remain Arduino and no sketches would be modified. It's just the source that would need some massaging for non-Arduino users.

@ttlappalainen
Copy link
Owner

Hi,

Thanks for comments. I'll about them. There has been discussion about same thing within othe issue.

@denravonska
Copy link
Contributor Author

I got it working on AVR32 with most of the changes above. What I haven't solved yet is how Stream should be replaced or wrapped. The debug uses could perhaps be removed and handled by the application instead. That leaves the forward stream in NMEA2000.cpp (also a debug tool?) and the SendInActisenseFormat function which only uses the write portion of Stream, unless I'm missing something. That could be replaced with a function pointer.

@thomasonw
Copy link
Contributor

thomasonw commented Dec 5, 2016

Hello - as ttlappalainen commented, I too have been working on 'normalizing' the current NMEA2000 lib - targeting the STM32 line of CPUs, presently using the MBED environment. Refer the last post in issue #28 Was not too hard to back out the Arduino specifics, and/or make compile time subs for them. But how to deal with the printing is a major issue (Currently I have simply commented them out).

So - going forward: It seems very achievable to bring the current lib into a more common C standard (And Timo, I am not aware of ANY other open-source full J1939 stack. Most the other NMEA2000 projects seems to rely on a Actisense reader, or are simple read-only - ala the Ford motor company effort). @ttlappalainen, are you interested in doing this? How should the printing be handled?

@ttlappalainen
Copy link
Owner

Note that denravonska is working with this. Some changes has been already published and some is on his table.

@denravonska
Copy link
Contributor Author

denravonska commented Dec 6, 2016

@thomasonw I have also fixed everything but the serial I/O. I have a solution for this but I'm not too happy with it since it forces the user to implement a bridge on the Arduino end to bridge the library and an Arduino Stream class:

class ArduinoN2kStreamBridge : public N2kStream {
public:
  ArduinoN2kStreamBridge(Stream& stream)
    : stream(stream) {
  }

  int read() {
    return stream.read();
  }

  size_t write(const uint8_t* data, size_t size) {
    return stream.write(data, size);
  }

  Stream& stream;
} StreamBridge(Serial);

We might not have much of a choice though. I'll keep working on it.

@denravonska
Copy link
Contributor Author

@thomasonw Could you try my branch on your platform? https://github.com/denravonska/NMEA2000/tree/portability-fixes
As long as you don't use the stream functionality you don't have to implement N2kStream.

@ttlappalainen
Copy link
Owner

ttlappalainen commented Dec 6, 2016 via email

@denravonska
Copy link
Contributor Author

@ttlappalainen You can use "Clone or download" to download a zip that can be imported to the Arduino IDE. If you want to test the lib on Arduino you do not have to implement N2kStream. It works as it does in your repo: NMEA2000.SetForwardStream(&Serial);. The interface just "happens" to look the same as the Arduino Stream interface ;)

@thomasonw
Copy link
Contributor

Hello - so, overall there are perhaps three of challenges here to bring the current NMEA2000 lib into a more portable 'ANSI C' styles:

  1. Using more common 'C99' conventions. Ala, u8int_t vs. byte, 0x01<<3 vs B00000100, lowByte()... - this is somewhat simple and I think Danravonska you have many of them.
  2. Backing out the AVR specifics. Mostly macros for PROGMEM, and the associated _P() funcitons. I think some are covered, but a few more are needed. Again, rather achievable.
  3. Deal with printing.

This last one is tough. The issue is fundamentally: Arduino IDE uses print() and println() for their streams, while more common C used printf()...

For my testing I have been using the MBED environment, which I believe targets a C99 variant. And it is the print() and println() that is the show-stopper and what needs the most thought.

@denravonska : I downloaded your edits, but there are still several items in points 1 & 2 above to be addressed. Rather then create yet-another-forlk of the NMEA2000 lib, the attached .ZIP file is my current edits inside the MBED environment. (Note that I have simply commented out all printing..) I still do not have a clean compile (there are a few warnings), and still need to do work around the basic CAN support for the STM32F0 line of CPUs. But maybe this will give another idea of some of the other issues.

But more so: How to address print() and println() vs. printf()... THAT is the big issue here I think.

NMEA2000-ANSI_C-12-6-2016.zip

@denravonska
Copy link
Contributor Author

denravonska commented Dec 6, 2016

I have print and println covered in my branch, at least from the testing I've done. My idea was to get the portability stuff out of the way now and do minimal changes to the lib itself to get a stable foundation. Then we could focus on the internals. My idea was to tweak the N2kStream interface into maybe an N2kIO containing the following functions:

  • read
  • write
  • log

That way you can have the read and write handle the actual data while log takes care of debug logging. The logger could do printf syntax (log(const char* fmt, ...);). If your app does not need logging you could simply discard the log data.

Anyway, that was my idea. Minimal changes now to avoid modifications in the applications, restructuring later.

Edit: Actually, the lib builds for AVR32 and Arduino now. As soon as someone with hardware could test it on Arduino we could merge it. I haven't fixed any warnings though.

@thomasonw
Copy link
Contributor

thomasonw commented Dec 6, 2016 via email

@denravonska
Copy link
Contributor Author

denravonska commented Dec 6, 2016

@thomasonw The progmem thingys should be fixed in denravonska@6cf0e29 :) I'm all for fixing the rest. Once @ttlappalainen gives the PR a go, could you add your change requests to it?

@denravonska
Copy link
Contributor Author

denravonska commented Dec 6, 2016

Wait, the progmem is not fixed at all. I'll get right to it.

Edit: Well, it will work for Arduino but not for AVR in general.

@denravonska
Copy link
Contributor Author

I have pushed some additional fixes to my branch covering missing itoa implementations and additional PROGMEM support. I'm not too keen on this whole #ifdef situation, but it's inevitable to keep compatibility with Arduino, stray AVR8 and the rest. I have no solution for it in mind though.

@thomasonw
Copy link
Contributor

Hello. Thanks for the work! Quick scan, did notice a few #ifdef AVR, as well as abstraction of the avr type capabilities (e.g., PROGRAM being redefined as CDATA.) Think a different way might be to simple define the AVR specifics to null out on non AVR machines, ala:
#define PGM_P const char *
#define PSTR(str) (str)
#define memcpy_P(dest, src, num) memcpy((dest), (src), (num))
. . . .

Then the main code can remain clean, as well as keep the historical 'Arduino/AVR' names for things like PROGMEM in place.

Just an idea.

Will play some with the latest edits and see what happens.

@denravonska
Copy link
Contributor Author

denravonska commented Dec 7, 2016

@thomasonw Ah, that could be an option. My approach was to be able to use the same capabilities on non-AVR platforms. It could be that it's too much of an AVR thing as I haven't seen it elsewhere. I'll try your suggestion out and see how she rolls.

@denravonska
Copy link
Contributor Author

@thomasonw Fixed now. I don't like how it enforces the paradigms of one platform on the rest, but you are right that it is a whole lot cleaner and easier.

I couldn't get away from the ifdef in N2kStream since we will want to print constant strings on non-Arduino (but still AVR) targets.

@thomasonw
Copy link
Contributor

thomasonw commented Dec 8, 2016

@denravonska -- Today I was able to get a clean compile of your edits mocking up an ActisenseReader demo function under the MBED environment with only these changes:

  1. Adding functions for millis() and delay() using MBED calls.
  2. Replacing round() with floor()

It seems round() is not well supported, even in C99 'compatible' compilers/libs – I am thinking switching to floor() will not cause that many issues?

I have not yet finished a full working example of the reader demo under MBED using the STM32F07, but perhaps it is close. (FWIW, I am not at all impress with the web based MBED – no debug, VERY limited editor, on-line linked error ‘help’ tends to simply say “Fix It”. Plus, I have been locked out more than once with their online tools being not available or malfunctioning. ) If I have time the rest of the week I will work to get something functional, but we are looking to head out of town Saturday for the rest of the month – so not sure how much time I will have. But I will also try the modified lib under a Keil uVision, using its compiler and libs to see if any other portability issues come up - at least to the point of a clean compile...

@ttlappalainen - At this point where do you want to go with all this. It seems there is a proof here of the ability to have a bit more portability. Is this something you would like to move the main lib towards? If so, I think there are some coding style selections that you should look at - and perhaps if you want to look over everything and decide on a way to go that would be best?

Also - I think the NMEA2000_CAN.h file could get very messy quickly. Even now there seems to be two different directions for STM32 support, one using the stm32duino.com approach (the <MapleIntCompatibility.h> include.) And myself looking at MBED libs for the STM32F0. I might suggest: perhaps it is best to reserve the NMEA2000_CAN.h file only for true Arduino ports, or very-close ones like the stm32duino effort. For ports like my MBED, or Danravonska's targeting the basic Atmel AVR environment (I am assuming that is what you are doing). Maybe it is best to make us use the NEMA2000 libs the old way, without the NMEA2000_CAN.h file.

OK, tt - what all do you think of this..

@thomasonw
Copy link
Contributor

denravonska commented: My approach was to be able to use the same capabilities on non-AVR platforms. It could be that it's too much of an AVR thing as I haven't seen it elsewhere. I'll try your suggestion out and see how she rolls.

Ya, I know the AVR uCs need things like PROGMEM as they use totally different address spaces / access methods for FLASH/RAM. While with the ARM CPUs the address space is unified, and I think simple compiler clues such as const are sufficient to keep from repeating data in RAM..

@denravonska
Copy link
Contributor Author

denravonska commented Dec 8, 2016

@thomasonw Oh, yeah, round seems to be a thing of the past. We can change it to floor(val + 0.5f); for the same result. I'll push a fix for it today, or will you after the PR?

Regarding NMEA2000_CAN, I don't use it in AVR32 at all and it doesn't get in the way (by sheer luck?). My setup is:

class Nmea2000Reader : public tNMEA2000 {
   IO::ICanBusPtr m_Can; // My CAN driver
   ...
};

I suppose the NMEA2000_CAN is to allow the user to switch boards without having to change drivers, correct?

@thomasonw
Copy link
Contributor

@denravonska NMEA2000_CAN.h: Yes, idea is to do auto-select. The 'Old Way' is how you are doing it.

So, some more news Good and Bad. Just now I did some testing on my existing ATmega64M1 project. Using you lib I was able to compile, and the code size was reduced by around 10 bytes! (Always good). However, I was testing with debug enabled and no CAN connection - so I should be seeing error messages in the serial stream along the lines of:
PGN 131069 send failed
PGN 131068 send failed

However, I do not... So, there is something amiss with the serial out handling... Program does not hang, and other serial functions continue to work - just am not seeing the error messages from the NMEA2000 lib when placed into debug mode via:

     _NMEA2000.SetForwardType(tNMEA2000::fwdt_Text);                                // Send error and status messages to the Serial port._

Sorry, and turning in for the night now. If I get time tomorrow I will see if I can figure out anything - or maybe you will find something before then.

@ttlappalainen
Copy link
Owner

I definetely like portability!

I see the Arduino system is meant for simple startup for almost with no programming skills. So this is also why I made NMEA2000_CAN.h to keep startup simple and independent what board one uses. So I do not see it as problem to have different method for more embedding systems. So for Arduinos I would like to keep compatibility.

@ttlappalainen
Copy link
Owner

The inherited class CANSendFrame should return false, if it can not send frame and the send buffer becomes full.

@ttlappalainen
Copy link
Owner

Hopefully you did not use NMEA2000.SetDebugMode(tNMEA2000::dm_ClearText); or .. dm_Actisense); since with that it never tries to use CANSendFrame

@thomasonw
Copy link
Contributor

Here is my startup code. DEBUG is defined, and I am able to see status / error messages in the serial stream with the original lib. Do you see anything wrong here?


bool initialize_CAN(void) {
    
        NMEA2000.SetProductInformation(&AltRegProductInformation );                     // Define Product information

        NMEA2000.SetDeviceInformation(1,                                                // Unique number. Use e.g. Serial number.
                                      141,                                              // Device function=Alternator. See codes on http://www.nmea.org/Assets/20120726%20nmea%202000%20class%20&%20function%20codes%20v%202.00.pdf
                                       50,                                              // Device class=Propulsion.    See codes on  http://www.nmea.org/Assets/20120726%20nmea%202000%20class%20&%20function%20codes%20v%202.00.pdf
                                     1401                                               // Manuf Code: Just chose free from code list on http://www.nmea.org/Assets/20121020%20nmea%202000%20registration%20list.pdf
                                     );


        // NMEA2000.SetSingleFrameMessages(SingleFrameMessages);                           // Register our extended list of messages we will accept.
        // NMEA2000.ForwardOnlyKnownMessages(false);                                       // Have the NMEA2000 lib send us ALL non-system messages, we will sort out which ones we want to deal with.
                                                                                        //  WAIT, This is the DEFAULT setting!  So - - -  Do not really need to call this. 
        #ifdef DEBUG
          NMEA2000.SetForwardType(tNMEA2000::fwdt_Text);                                // Send error and status messages to the Serial port.
          #endif

        NMEA2000.SetMode(tNMEA2000::N2km_NodeOnly,128);                                 // Configure for normal node, start looking for dynamic addresses @ 128 (Preferred "Power Components" range in RV-C spec)
        NMEA2000.EnableForward(false);                                                  // Do not forward CAN messages to the Serial port.
        NMEA2000.SetN2kCANMsgBufSize(2);                                                // Define only 2x reception buffers - should be sufficient.
        NMEA2000.SetMsgHandler(handle_CAN_Messages);                                    // Callback function NMEA2000.ParseMessages() uses when a CAN message is received.
        NMEA2000.SetISORqstHandler(handle_CAN_Requests);                                // Callback function NMEA2000.ParseMessages() uses when an ISO Request is received.
        NMEA2000.Open();                                                                // And start up the CAN controller.

        return(true);
}

@ttlappalainen
Copy link
Owner

Looks OK.

So with same code you get errors from my current master lib, but not from the "portable" lib under development?

Could it be something with new N2kStream implementation?

@denravonska
Copy link
Contributor Author

@thomasonw Serial is no longer the default forward stream. I had to remove that in order to remove the Arduino dependency. You can set it using NMEA2000.SetForwardStream(&Serial);.

@thomasonw
Copy link
Contributor

thomasonw commented Dec 8, 2016

@denravonska ok! (Still up, but not for long). That solved it. Portable lib now sending out messages. Will leave it between you and ttlappalainen on the change to the API : - )

Overall good job! Looking forward to getting things polished up and into the main lib.

@denravonska
Copy link
Contributor Author

I pushed a fix regarding the non standard round() function (or macro on Arduino). It compiles for me on AVR32 and in the Arduino IDE but it could be good if someone else gave it a shot.

@ttlappalainen Are you ok with dropping Serial as the default forward stream?

@ttlappalainen
Copy link
Owner

I'll test it later - maybe goes tomorrow.

It is ok dropping serial. We just need to mention it on changes.

@denravonska
Copy link
Contributor Author

@thomasonw You have a very picky compiler. It requires the functions to be declared virtual when they are virtual in the parent. That should probably fix the rest of the errors as well.

@thomasonw
Copy link
Contributor

thomasonw commented Dec 10, 2016

@denravonska Thanks, that took care of the implicit victuals. Any idea about:

tNMEA2000_smt32::tNMEA2000_smt32() : tNMEA2000() { }

I am still getting compiler errors:

..\..\..\..\Desktop\NMEA2000-portability-fixes\NMEA2000_stm32.cpp(38): error: #276: name followed by "::" must be a class or namespace name tNMEA2000_smt32::tNMEA2000_smt32() : tNMEA2000() { ..\..\..\..\Desktop\NMEA2000-portability-fixes\NMEA2000_stm32.cpp(38): error: #260-D: explicit type is missing ("int" assumed) tNMEA2000_smt32::tNMEA2000_smt32() : tNMEA2000() { ..\..\..\..\Desktop\NMEA2000-portability-fixes\NMEA2000_stm32.cpp(38): error: #130: expected a "{" tNMEA2000_smt32::tNMEA2000_smt32() : tNMEA2000() { ..\..\..\..\Desktop\NMEA2000-portability-fixes\NMEA2000_stm32.cpp(38): warning: #940-D: missing return statement at end of non-void function "tNMEA2000_smt32" } ..\..\..\..\Desktop\NMEA2000-portability-fixes\NMEA2000_stm32.cpp: 1 warning, 3 errors

If I comment out that line, I am able to get a clean compile/link - have to admit, I am not fully sure what that line is trying to do... @ttlappalainen can you help?

@denravonska
Copy link
Contributor Author

That one calls the constructor of the parent, but it's called anyway so you can remove it.

@thomasonw
Copy link
Contributor

Well, that did not work - caused linking errors. However, making sure I had the correct spelling did solve all! have compiled / linked / uploaded in MBED. Compiled/linked in Keil. Still more to do, ActisenseReader demo is not running yet. But closer...

(BTW, I think doing a test for Raspberry Pie would be worthwhile to verify the 'portability' of this..)

@ttlappalainen
Copy link
Owner

Have you tried to have just on NMEA2000_stm32.h:
public:
tNMEA2000_smt32() : tNMEA2000() { }

And what about name is it really tNMEA2000_smt32() not tNMEA2000_stm32 as you .ccp is named?

@denravonska
Copy link
Contributor Author

@thomasonw I think this is a minor typo or error in the code, not in the portability so we can take the tracking off-thread. Can you mail your current code to med and I'll try to help you.

@thomasonw
Copy link
Contributor

thomasonw commented Dec 11, 2016 via email

@thomasonw
Copy link
Contributor

@denravonska @ttlappalainen Hello. I am back focusing on the STM32F072 porting using the mbed dev environment - and am getting close, but still a couple of issues:

  1. I had to make one fix, the removal of " tN2kPGNList::" and just use the constants directly. You can see the changes here (I am holding off doing a push until things are confirmed).
    https://github.com/thomasonw/NMEA2000/commit/6a15824530ddc4abef242fbc03aec74fdea1c76e

  2. I am having issues bridging the mbed serial into what the NMEA2000 lib wants. Here is the test code:

#include "mbed.h"
#include "NMEA2000_stm32.h"

Serial pc(SERIAL_TX, SERIAL_RX, 115200);

DigitalOut myled(LED1);

tNMEA2000_stm32 NMEA2000;

int main()
{
    pc.printf("\r\n\nAbout to start\r\n");
    pc.print("Will this work??\r\n");
    pc.println("No sure");
    
    
    NMEA2000.SetForwardStream(&pc);                                 // PC output on due native port
    NMEA2000.SetDebugMode(tNMEA2000::dm_ClearText);                 // Lets us see the debug mesages on the terminal
    NMEA2000.SetForwardType(tNMEA2000::fwdt_Text);                  // Show in clear text
    NMEA2000.Open();

    while(1) {
        NMEA2000.ParseMessages();
        myled = 1; // LED is ON
        wait(0.2); // 200 ms
        myled = 0; // LED is OFF
        wait(1.0); // 1 sec
    }
}

and here are the error messages I get:

Warning: List initialization syntax is a C++11 feature in "NMEA2000-master/N2kMsg.cpp", Line: 370, Col: 7
Warning: Dynamic initialization in unreachable code in "NMEA2000-master/NMEA2000.cpp", Line: 916, Col: 8
Error: Class "mbed::Serial" has no member "print" in "main.cpp", Line: 13, Col: 9
Error: Class "mbed::Serial" has no member "println" in "main.cpp", Line: 14, Col: 9
Error: Argument of type "mbed::Serial *" is incompatible with parameter of type "N2kStream *" in "main.cpp", Line: 17, Col: 32

The largest one being the last one: incompatible parameter of N2Kstream with mbed::Serial - if I cast I am able to get it to compile, but of course it does not function. The other two Error:'s are to simply show that the mbed::Serial does not support print nor printf

I am a pre-objects kind of guy, so these issues are tough for me to overcome. Help? If you need to see the rest of the code, I can figure out how to place it somewhere. On a plus side, if I pass in NULL for the stream - the code will compile, load, and flash the LED. I have not been able to test it yet with another CAN device, soon.

@denravonska
Copy link
Contributor Author

@thomasonw You need to make your own class which derives from N2kStream and wraps an mbed Stream. Something like (not tested):

class mbedStream : public N2kStream {
   mbedStream()
      : pc(SERIAL_TX, SERIAL_RX, 115200) {
   }
   
   int read() {
      return pc.getc();
   }
   
   size_t write(const uint8_t* data, size_t size) {
      return pc.write(data, size);
   }
   
   Serial pc;
};

mbedStream serStream;
NMEA2000.SetForwardStream(&serStream);

The NMEA200 library implements the missing print functions in N2kStream so your serStream will have them.

@thomasonw
Copy link
Contributor

@denravonska THANKS!

With that starting point I was able to get wrappers working, The NMEA2000 lib is now complaining loudly it is unable to send any CAN messages (Mostly because there is no transceiver connected at this point!) - So: Printing works! Will be a week or so until I can test the actual CAN code, but have not too many concerns there.

Thanks again! Will post up somewhere the final code for your review after I get the rest working.

@thomasonw
Copy link
Contributor

@denravonska @ttlappalainen All, wanted to pass on I have been working on a RPi bridge and just today posted my 1st cut here: https://github.com/thomasonw/NMEA2000_socketCAN

I have it working with a simple read demo, still need to verify the write.

It used socketCAN for this RPi bridge so it should be very portable to any linux system which has socketCAN enabled hardware.

Timo: Should this be added to the 'automatic' selections - thinking as socketCAN is a very common 'portable' system might be worthwhile to do so?

@denravonska
Copy link
Contributor Author

@thomasonw That's pretty neat. We've discussing using the Pi for one of our projects so we might give it a go.

@thomasonw
Copy link
Contributor

Nice - of course pass on anything you might find : - ) I am looking to use it for a signal-K provider from J1939 based protocols (Primary RV-C, but some NMEA2000 might come along as well)

@olesenrl
Copy link

olesenrl commented Mar 4, 2017 via email

@ttlappalainen
Copy link
Owner

Automation for SocketCAN would be nice feature. Would there be a problem for selection rules? There should be also new force definition #define USE_N2K_CAN 5 // for use with SocketCAN.

Be aware with delay() on CANSendFrame. That will destroy sending. NMEA2000 library itself has buffering, so if socketCAN can not send, it would be better to return false, so frame will be buffered. Note that wait_sent should be actually block_sent, which means that system should not use different "streams" on controller. Like with MCP2515 controller there is possiblity send fast packet frames in wrong order, if you do not use block_sent and single send buffer.

Could you have your NMEA2000_socketCAN also with MIT license?

What CAN bus controller that uses?

I also would like to have simple instructions how to start with scratch with RPi as I have for Arduinos. I need them also for myself, since I have been too busy and lazy to start with RPi.

@thomasonw
Copy link
Contributor

thomasonw commented Mar 5, 2017

OK, changes pushed up.

  1. Changed lic to MIT
  2. Removed retry in send()
  3. Added some additional diag messages to cerr

I am not clear what block_sent should be doing, and am ignoring it. Do I need to dog into it more and make sure there is not an issue with socketCAN as well as the AVR controller?

I will type of more detailed RPI guide and put it into the read.md file. In summary, most of the stuff is already installed – just needs to be enabled / configured. The CAN adapter I am using is a PiCAN 2 board http://skpang.co.uk/catalog/pican2-duo-canbus-board-for-raspberry-pi-23-p-1480.html I selected it mostly because it has a nice fit to the RPi formfactor, but there is nothing special about it – just a simple MCP2515 adapter --> $2 or so on Ebay. (Does this come back to the block_sent question?)

Another option that I think would work well is a can-usb controller. Googling "socketCAN USB" shows a lot of options, and I see a few available in Ebay for around $20. Would allow desktop / laptop Linux machines easy access to the NMEA2000 lib work.

I think the compile ID #ifdef __linux__ will work for auto select. At this point I have been keeping my fork of NMEA2000 lib down-rev as the main stream has grown a bit in size and I can not longer fit the current rev and my code into the small ATmega64M1 : - ( But will see if there is a way I can create a push for the auto select.

Do help me understand a bit about the block_send parameter and I will see if changes are needed.

@thomasonw
Copy link
Contributor

Oh, also changed the lic on NMEA2000_avr to MIT -- when did the changes go into the main NMEA2000 lib to deal with sending retries? (I noted you took out the retry loop in the Due code in Sept 2016)

@thomasonw
Copy link
Contributor

@ttlappalainen : I was able to add the socketCAN as type 5 and create a pull request, see: #41

Looks like the pull was merged with another to fix an MBED compiler error.

@ttlappalainen
Copy link
Owner

MCP2515 controller has 3 buffers for sending. So if you start to send fast packet from begining, frames may be written 1->buf1, 2->buf2, 3->buf3. As far as I have understood due to timing it may be that controller transmit prioritising sends your frames order 2,3,1. Not all (if any) systems can handle fast packet frames provided on wrong order.

In mcp_can this has been solved so that if wait_sent is true, it polls buffer transmit bit until it has been send. In current code it has 10 us delay between each poll, which also causes some problems. I tried to change it to use interrupt also for sending, but could not get it work yet. With interrupt it would be possible to use only single buffer for sending in case of wait_send is true.

@thomasonw
Copy link
Contributor

thomasonw commented Mar 5, 2017 via email

@ttlappalainen
Copy link
Owner

In library function SendMsg I set wait_sent true for fast packet frames and false for others.

@thomasonw
Copy link
Contributor

OK, ref revised approach - still not great, but better.

I have debugged CANSendFrame() is tested it using one of the examples. Works well.

wait_send flag is also handled, but by putting the code to sleep for 2mS to allow for time for the message to be sent. I have not been able to locate a way to assure keeping queue order in socketCAN, nor for a way to check if the Tx queue is empty - so using this for now. (If any ideas??) I do have the issue out to some message boards and will revise this code if a better approach is found.

@ttlappalainen
Copy link
Owner

As I told I tried to change mcp_can to use interrupt also for sending. With first try it stuck after few send, so needs some more thinking. So the only solution anyway would be to dig a bit deeper to socketCAN and improve it as well mcp_can. In 2 ms you may have 6 frames on the bus (250000 b/s / 10 b/B / 8 B/f * 0.002 s), which you will then loose exept, if you have big enough interrupt handled internal receiving buffer in socketCAN.

@thomasonw
Copy link
Contributor

thomasonw commented Mar 9, 2017

Hi - socketCAN is all IRQ driven with typically large buffers, so no lost messages. The 2mS sleep (sleep --> allows other programs to run, just not us for 2mS) is clearly a patch, and should work OK, providing there are not too many messages on the bus at the same time.

There seems to be an approach whereby one opens a 2nd socket and looks via it for reception of the sent message - which would mean it did indeed get out to the CAN bus. That might be the final answer for socketCAN - need more research; for now think the sleep() will lower any risk of out of sequence messages.

@thomasonw
Copy link
Contributor

FYI, I just reviewed the _avr CAN code. It only uses one of the mailboxs for Tx (default - can be changed, but is not for the NMEA2000 stuff), so no issue with out-of-order in the hardware. the IRQ drive queue is a true FIFO, so again no issues with out of order. Was good to be able to review this code again.

@thomasonw
Copy link
Contributor

thomasonw commented Mar 15, 2017

@ttlappalainen: Just now I pushed up a modified NMEA2000_socketCAN here: https://github.com/thomasonw/NMEA2000_socketCAN

I have been looking into the issue of keeping packet order and am currently under the conclusion that socketCAN does that by default - so no extra steps are needed. I am basing this on review of the socketCAN teamroom - specifically a series of posts in the 2008/2010 timeframe addressing out-of-order issues, as well as reviewing the code for a limited number of devices. I have NOT received positive confirmation from that same development treamroom, but will also note it appears to have had little activity in the past 2 years.

For now I am suggesting to proceed forward with this latest push, I will continue to see a definitive answer and if there is any change / news will report back.

Other systems: I have confirmed nmea2000_avr has no out-of-order issues due to a single Tx mailbox an and a true interrupt fifo buffer. It should stable.

Yesterday I was able to demo the ported lib using MBED (ARM micro-controllers). I still need to do some work, and one of the key issues is there is no specific focus on keeping packets in order inside the MBED libs, so I still need to resolve that. After I finalize that port will post it to my github site and let you know. (Or, and I will be looking to add the MBED environment into the auto-selection as well, but will wait to make a pull request until I compete the bridge lib).

@denravonska : SO, this make 3x 'non-Arduino' envirs that your works has enabled! Great!

@ttlappalainen
Copy link
Owner

So does it mean that I should change header "NMEA2000 library for Arduino" to something new? E.g. "NMEA2000 library for microsystems"? Succestions?

@thomasonw
Copy link
Contributor

Maybe. It does still have primary an 'Arduino' feel to it, but given we are now running in Arduino (several boards). MBED (ARM), Linux (including RPi), and Atmel environments - does seem a more inclusive name would be appropriate.

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 a pull request may close this issue.

4 participants