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

Parport build test #1874

Merged
merged 2 commits into from
Aug 24, 2024
Merged

Parport build test #1874

merged 2 commits into from
Aug 24, 2024

Conversation

ndim
Copy link
Contributor

@ndim ndim commented Aug 8, 2024

This came up during build tests: The parallel port code was not built during CI, and it failed compiling for Windows using the mingw64 or ucrt64 cross compile toolchains on Fedora Linux.

So this PR

  • fixes the function prototypes for port_get() in ppiwin.c to allow compilation

  • has all platforms on GitHub's CI infrastructure on which avrdude supports the parallel port test build with the parallel port code

It does not fix the fact that libavrdude uses a avrdude_message2() function which is defined in avrdude, which causes problems building for Windows. A library using a symbol from an executable while the same executable uses a symbol from the library appears to be difficult to impossible with Windows DLLs.

@stefanrueger
Copy link
Collaborator

Thanks for the PR!

problems building for Windows

How has this been built until now? Statically without DLL?

I have thought for some time that main.c isn't the best place for the messaging functions. Sounds like it's better to move them to a different place that could be compiled into a messaging library? That way someone can build their own all-singing/all-dancing messaging library and instead link to that? Might also be the way for Jörg's GUI #1714? @dl8dtl? Not sure this can happen before v8.0 but if people think this is a priority we could give it a go.

@ndim
Copy link
Contributor Author

ndim commented Aug 8, 2024

I have not looked into what exactly the CI builds (Windows+MSYS2/mingw or Windows/msvc) do, but statical linking should work.

For dynamic linking and message output functions, a usual pattern is that a library defines a C function type and offers an initialization function to its library users which takes a function of such type and some data to be handed to such a function.

So a library user like avrdude or avrgui registers its respective message handler with libavrdude when it initializes the library. Then avrdude's message handler be something like its current avrdude_message_2() and print to stdout, and avrgui's message handler would be a function which writes to a GUI log window.

A reasonable default implementation for such a message handler could be printing to stdout.

/* libfoo.h */
void (*message_handler_func)(void *data, char *msg);
void register_message_handler(message_handler_func *handler, void *data);
/* libfoo.c */
static void default_message_handler(void *, char *msg)
{
  fputs(msg, stdout);
}

static message_handler_func *message_handler = default_message_handler;
static void *message_handler_data = NULL;

static void foo_message(char *msg)
{
  message_handler(data, msg);
}

void foo_bar(...)
{
  foo_message("doing bar");
  ...
}
/* gui-application-using-libfoo.c */
void message_handler(void *data, char *msg)
{
  MainWindow *main_win = (MainWindow *)data;
  main_win.show_message(msg);
}

The respective message handler data and message handler func could be part of an opaque library context type struct.

Details to be determined.

@mcuee
Copy link
Collaborator

mcuee commented Aug 8, 2024

Please forget about Parport in Windows, it is not supported and will not work as there is no driver support.

@ndim
Copy link
Contributor Author

ndim commented Aug 8, 2024

Or avrdude_message2() could be moved from the avrdude executable into the libavrdude library. But unlike e.g. the warning messages Gtk GUIs print to the console, the messages printed through avrdude_message2() can be essential for the user to see, so they would need to be made available to a user in some way.

@ndim
Copy link
Contributor Author

ndim commented Aug 8, 2024

Please forget about Parport in Windows, it is not supported and will not work as there is no driver support.

ppiwin.c does not work on Windows? What was it written for, then? 16 bit Windows?

@mcuee
Copy link
Collaborator

mcuee commented Aug 8, 2024

Please forget about Parport in Windows, it is not supported and will not work as there is no driver support.

This has been documented under the Wiki here.
https://github.com/avrdudes/avrdude/wiki/Known-limitations-of-avrdude#known-issues-for-programmers

Parallel port based programmer will nork under Windows and macOS. The old version of avrdude ships with giveio.sys driver which may work under Windows XP but not later version of Windows. It was removed in the following PR.
#769

@stefanrueger
Copy link
Collaborator

avrdude_message2() could be moved [to] libavrdude [...] would need to be made available to a user in some way

I like this idea. avrdude_message2() could be a function pointer that can be overwritten by the user of that library. @dl8dtl?

@ndim
Copy link
Contributor Author

ndim commented Aug 8, 2024

So it would make sense to remove ppiwin.c and have the builds fail in whatever way when given cmake -D HAVE_PARPORT or configure --enable-parport.

@mcuee
Copy link
Collaborator

mcuee commented Aug 8, 2024

So it would make sense to remove ppiwin.c and have the builds fail in whatever way when given cmake -D HAVE_PARPORT or configure --enable-parport.

I like this idea.

@mcuee
Copy link
Collaborator

mcuee commented Aug 8, 2024

I have mentioned this as part of Docuementation improvement.

ndim added a commit to ndim/avrdude that referenced this pull request Aug 8, 2024
avrdude does not support parallel port usage on Windows XP or later,
so we can remove winppi.c.

avrdudes#1874 (comment)
@mcuee mcuee added the enhancement New feature or request label Aug 8, 2024
@mcuee
Copy link
Collaborator

mcuee commented Aug 8, 2024

@dl8dtl
Just want to double confirm with you to check if you are okay to remove Parallel Port under Windows. Thanks.

I look at the code and I do not think it will work under Windows Vista and later.

And we do not support Windows XP/Vista anyway. Even Windows 7 support is not tested now that it is no longer supported by Microsoft.

ndim added a commit to ndim/avrdude that referenced this pull request Aug 8, 2024
Build parallel port code on supported systems

Unsupported systems are:

  * MSVC compiler (does not support GNU style inline asm)
  * macos operating system (no macos implementations for preprocessor
    macros DO_PPI_READ, DO_PPI_WRITE, ppi_claim, ppi_release)

This means the CI can test

  * native Linux builds (yes, even for arm systems)
  * native Windows builds using mingw

Untested at this time are the BSDs.

Removes the dysfunctional Windows (since Windows XP)  parallel port code,
and has the buildsystems fail Windows builds if parallel port builds are
requested (HAVE_PARPORT or --enable-parport).

avrdudes#1874 (comment)
@dl8dtl
Copy link
Contributor

dl8dtl commented Aug 12, 2024

avrdude_message2() could be moved [to] libavrdude [...] would need to be made available to a user in some way

I like this idea. avrdude_message2() could be a function pointer that can be overwritten by the user of that library. @dl8dtl?

I think I could live with that.

Just want to double confirm with you to check if you are okay to remove Parallel Port under Windows.

I don't think there was any actual parallel-port support for Windows available anyway since the days are gone when you could gain access to all IO ports through giveio.sys.

@mcuee
Copy link
Collaborator

mcuee commented Aug 12, 2024

Just want to double confirm with you to check if you are okay to remove Parallel Port under Windows.

I don't think there was any actual parallel-port support for Windows available anyway since the days are gone when you could gain access to all IO ports through giveio.sys.

Great. Thanks for your confirmation. This is what I believe as well.

@mcuee
Copy link
Collaborator

mcuee commented Aug 20, 2024

@ndim

Actually this PR is kind of good already. Just wondering any further changes you want to have. Thanks.

@ndim
Copy link
Contributor Author

ndim commented Aug 20, 2024

The commit comment is incorrect at least regarding Windows. Doc changes have been done in other PRs, expecting conflicts.

But you are right, this PR could be made merge able with probably low effort. Will try later today.

mcuee added a commit that referenced this pull request Aug 21, 2024
@mcuee
Copy link
Collaborator

mcuee commented Aug 22, 2024

@ndim

I have committed the document changes about Windows parallel port into git main. I also found another place mentioning Windows parallel port and I removed them as well. Thanks.

@mcuee
Copy link
Collaborator

mcuee commented Aug 23, 2024

@ndim

Please help to rebase this PR and fix the commit messages, then I think it will be good to go.

mingw builds for Windows require a function prototype fix.
Build parallel port code on supported systems

Unsupported systems are:

  * Windows operating system
  * MacOS operating system

This means the CI can test

  * native Linux builds (yes, even for arm systems)

Untested at this time are the BSDs.

Removes the dysfunctional Windows (since Windows XP)  parallel port code,
and has the buildsystems fail Windows builds if parallel port builds are
requested (HAVE_PARPORT or --enable-parport).

avrdudes#1874 (comment)
@ndim ndim marked this pull request as ready for review August 24, 2024 13:47
@ndim
Copy link
Contributor Author

ndim commented Aug 24, 2024

Good to go, as far as I can tell.

@stefanrueger
Copy link
Collaborator

Great, will merge soon

@stefanrueger stefanrueger merged commit ca6e079 into avrdudes:main Aug 24, 2024
13 checks passed
@ndim ndim deleted the parport-build-test branch August 24, 2024 21:14
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.

4 participants